Cranelift: Behaviour of iadd_imm.i128 with negative constants

[afonso360]

👋 Hey, the clif fuzzer reported this issue.

.clif Test Case

test interpret
test run
set enable_llvm_abi_extensions=true
target x86_64
target aarch64


function %a(i128) -> i128 {
block0(v0: i128):
    v1 = iadd_imm.i128 v0, -1
    return v1
}
; run: %a(1) == 0

Steps to Reproduce

• clif-util test ./the-above.clif

Expected Results

I don't know.

The iadd_imm.i128 takes a i64 constant.

The interpreter sign extends the constant, the backend does not.

Actual Results

Interpreter sign extends and returns 0 passing the test.

Native returns 0x1_0000_0000_0000_0000 and fails.

Versions and Environment

Cranelift version or commit: main
Operating system: windows
Architecture: x86

Extra Info

I'm pretty much seeking confirmation that we don't sign extend these constant values. I think it looks weird that we input -1, but get something completely different.

cc: @cfallin @jameysharp

[bjorn3]

Backends should either sign extend, or the operand type should change to Uimm64 (which doesn't exist yet, unlike for some other sizes) I think.

[akirilov-arm]

Backends actually never see the iadd_imm instruction. Looking at the operation definition, an issue that springs to me straight away is that the operation is defined for I128, but the immediate value is only a 64-bit one, and there is no rule how to handle that. So, IMHO the first step is to decide how to extend the value and change the operation description accordingly, and then update the rest of the code. I don't expect any backend changes at all.

[afonso360]

The InstBuilder function for iadd_imm suggests that it should be sign extended, since it takes a Imm64 (
I think this is what @bjorn3 was alluding to.), unlike some operations that take a Uimm* argument which would signify a zero extend. (i.e. heap_addr, insert_lane).

But it looks like a use case that probably wasn't considered when adding these instructions initially so we should probably review this.

[akirilov-arm]

My point is that the behaviour should be specified explicitly, and yes, I agree that sign-extending makes the most sense.

[cfallin]

I agree that sign-extension makes the most sense here. There is actually an equivalent issue with iconst.i128; I just verified now that on x86-64 and aarch64, iconst.i128 -1 produces a zero-extended-i64 value (0x0000_0000_0000_0000_ffff_ffff_ffff_ffff); we should do the same thing in that case.