Stream: git-wasmtime

Topic: wasmtime / issue #4568 Cranelift: Behaviour of `iadd_imm....

Wasmtime GitHub notifications bot (Jul 31 2022 at 20:38):

afonso360 opened issue #4568:

:wave: 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

Wasmtime GitHub notifications bot (Jul 31 2022 at 20:38):

afonso360 labeled issue #4568:

:wave: 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

Wasmtime GitHub notifications bot (Jul 31 2022 at 20:38):

afonso360 labeled issue #4568:

:wave: 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

Wasmtime GitHub notifications bot (Jul 31 2022 at 20:41):

bjorn3 commented on issue #4568:

Backends should either sign extend, or the operand type should change to Uimm64 I think.

Wasmtime GitHub notifications bot (Jul 31 2022 at 20:41):

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.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:08):

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 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.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:19):

afonso360 commented on issue #4568:

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_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.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:20):

afonso360 edited a comment on issue #4568:

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.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:24):

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.

Wasmtime GitHub notifications bot (Aug 09 2022 at 17:57):

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.

Wasmtime GitHub notifications bot (Aug 15 2022 at 18:08):

cfallin closed issue #4568:

:wave: 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

Last updated: Nov 22 2024 at 17:03 UTC