afonso360 opened issue #4568:
:wave: Hey, the clif fuzzer reported this issue.
.clif
Test Casetest 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: x86Extra 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
afonso360 labeled issue #4568:
:wave: Hey, the clif fuzzer reported this issue.
.clif
Test Casetest 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: x86Extra 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
afonso360 labeled issue #4568:
:wave: Hey, the clif fuzzer reported this issue.
.clif
Test Casetest 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: x86Extra 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 commented on issue #4568:
Backends should either sign extend, or the operand type should change to Uimm64 I think.
bjorn3 edited a comment on issue #4568:
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 commented on issue #4568:
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 forI128
, 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 commented on issue #4568:
The InstBuilder function for
iadd_imm
suggests that it should be sign extended, since it takes aImm64
(
I think this is what @bjorn3 was alluding to.), unlike some operations that take aUimm*
argument which would signify a zero extend. (i.e. heap_add, 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.
afonso360 edited a comment on issue #4568:
The InstBuilder function for
iadd_imm
suggests that it should be sign extended, since it takes aImm64
(
I think this is what @bjorn3 was alluding to.), unlike some operations that take aUimm*
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 commented on issue #4568:
My point is that the behaviour should be specified explicitly, and yes, I agree that sign-extending makes the most sense.
cfallin commented on issue #4568:
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.
cfallin closed issue #4568:
:wave: Hey, the clif fuzzer reported this issue.
.clif
Test Casetest 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: x86Extra 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
Last updated: Dec 23 2024 at 12:05 UTC