Stream: git-wasmtime

Topic: wasmtime / Issue #2860 Cranelift: Panic compiling iadd_ca...

Wasmtime GitHub notifications bot (Apr 27 2021 at 02:19):

Hypersonic opened Issue #2860:

Hi! While using Cranelift as a backend for a project, I ran into a panic compiling iadd_carry instructions on x86_64. Please let me know if there is any other information I can provide that would be helpful.

.clif Test Case

function u0:0() fast {
block0:
    v1465 = iconst.i32 0
    v1467 = bconst.b1 false
    v1468 = iconst.i32 0
    v1469, v1470 = iadd_carry v1465, v1468, v1467
    v1476 = bint.i8 v1470
    v1356 = iconst.i64 0
    v1391 -> v1356
    v1423 -> v1356
    v1447 -> v1356
    v1464 -> v1356
    v1494 -> v1356
    v1516 -> v1356
    v1544 -> v1356
    store notrap aligned v1476, v1356+274
    trap user0
}

Steps to Reproduce

cargo run -p cranelift-tools -- compile test.clif --target x86_64

Expected Results

Code successfully generates without a panic.

Actual Results

Panic: thread 'main' panicked at 'ALU+imm and ALU+carry ops should not appear here!', cranelift/codegen/src/isa/x64/lower.rs:5740:13

Versions and Environment

Cranelift version or commit: main HEAD (4a830b at time of writing)

Operating system: Linux

Architecture: x86_64

Wasmtime GitHub notifications bot (Apr 27 2021 at 02:19):

Hypersonic labeled Issue #2860:

Hi! While using Cranelift as a backend for a project, I ran into a panic compiling iadd_carry instructions on x86_64. Please let me know if there is any other information I can provide that would be helpful.

.clif Test Case

function u0:0() fast {
block0:
    v1465 = iconst.i32 0
    v1467 = bconst.b1 false
    v1468 = iconst.i32 0
    v1469, v1470 = iadd_carry v1465, v1468, v1467
    v1476 = bint.i8 v1470
    v1356 = iconst.i64 0
    v1391 -> v1356
    v1423 -> v1356
    v1447 -> v1356
    v1464 -> v1356
    v1494 -> v1356
    v1516 -> v1356
    v1544 -> v1356
    store notrap aligned v1476, v1356+274
    trap user0
}

Steps to Reproduce

cargo run -p cranelift-tools -- compile test.clif --target x86_64

Expected Results

Code successfully generates without a panic.

Actual Results

Panic: thread 'main' panicked at 'ALU+imm and ALU+carry ops should not appear here!', cranelift/codegen/src/isa/x64/lower.rs:5740:13

Versions and Environment

Cranelift version or commit: main HEAD (4a830b at time of writing)

Operating system: Linux

Architecture: x86_64

Wasmtime GitHub notifications bot (Apr 27 2021 at 02:19):

Hypersonic labeled Issue #2860:

Hi! While using Cranelift as a backend for a project, I ran into a panic compiling iadd_carry instructions on x86_64. Please let me know if there is any other information I can provide that would be helpful.

.clif Test Case

function u0:0() fast {
block0:
    v1465 = iconst.i32 0
    v1467 = bconst.b1 false
    v1468 = iconst.i32 0
    v1469, v1470 = iadd_carry v1465, v1468, v1467
    v1476 = bint.i8 v1470
    v1356 = iconst.i64 0
    v1391 -> v1356
    v1423 -> v1356
    v1447 -> v1356
    v1464 -> v1356
    v1494 -> v1356
    v1516 -> v1356
    v1544 -> v1356
    store notrap aligned v1476, v1356+274
    trap user0
}

Steps to Reproduce

cargo run -p cranelift-tools -- compile test.clif --target x86_64

Expected Results

Code successfully generates without a panic.

Actual Results

Panic: thread 'main' panicked at 'ALU+imm and ALU+carry ops should not appear here!', cranelift/codegen/src/isa/x64/lower.rs:5740:13

Versions and Environment

Cranelift version or commit: main HEAD (4a830b at time of writing)

Operating system: Linux

Architecture: x86_64

Wasmtime GitHub notifications bot (Apr 27 2021 at 03:41):

iximeow commented on Issue #2860:

using this as an excuse to (finally) familiarize myself with the MachInst backend, i'm not sure why ALU+imm and ALU+carry ops should not appear here should be the case - it looks like x64/lower.rs is _the_ place for lowering to happen in x64, eschewing codegen/meta's lowering?

and if that's so, i'd expect rustc_cranelift_codegen to have hit this, but maybe not, since this is the only carry-shaped instruction i see over there.

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:15):

cfallin commented on Issue #2860:

Greetings @Hypersonic and thanks for the report!

The plan we made when starting to design the new backend was to phase out add+carry (and sub+borrow) opcodes in the CLIF, instead directly providing the wider adds/subs that one would normally lower into carry-using ops. In the legalization-based old backend, i128.add became an i64.add and an i64.add_carry; now we just open-code the two-add sequence. (This was in discussions between myself, @julian-seward1 and @bnjbvr back in Jan 2020 or so.)

The reason for this design choice was that representing carry flags as normal values is fragile performance-wise: to get the correct (fast) machine code, which implicitly passes the bool in the carry flag, one has to pattern-match on the two instructions. The case where the b1 is separate -- as in your example, coming from a constant -- has to be lowered separately with pushf or setc to extract the flag and popf or a conditional diamond with clc/stc. The latter codepaths would almost never be used and so are a correctness risk as well; and these instructions are slow, to the point that there should (hopefully!) almost always be a better way to solve the problem.

Given that we just recently switched the default backend and haven't started the work of removing the old one, we haven't yet cleaned up the opcode space, but we intend to do so eventually.

That does leave the question: what if one actually wants to generate an add-with-carry? I'm definitely open to understanding use-cases better. Are you hoping to take an arbitrary b1 and feed it into the add? Or is this part of a lowering of some other wider arithmetic operation? If the latter, is there any other instruction that would help?

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:16):

cfallin edited a comment on Issue #2860:

Greetings @Hypersonic and thanks for the report!

The plan we made when starting to design the new backend was to phase out add+carry (and sub+borrow) opcodes in the CLIF, instead directly providing the wider adds/subs that one would normally lower into carry-using ops. In the legalization-based old backend, i128.add became an i64.add and an i64.add_carry; now we just open-code the two-add sequence. (This was in discussions between myself, @julian-seward1 and @bnjbvr back in Jan 2020 or so.)

The reason for this design choice was that representing carry flags as normal values is fragile performance-wise: to get the correct (fast) machine code, which implicitly passes the bool in the carry flag, one has to pattern-match on the two instructions. The case where the b1 is separate -- as in your example, coming from a constant -- has to be lowered separately with pushf or setc to extract the flag and popf or a conditional diamond with clc/stc to set it. The latter codepaths would almost never be used and so are a correctness risk as well; and these instructions are slow, to the point that there should (hopefully!) almost always be a better way to solve the problem.

Given that we just recently switched the default backend and haven't started the work of removing the old one, we haven't yet cleaned up the opcode space, but we intend to do so eventually.

That does leave the question: what if one actually wants to generate an add-with-carry? I'm definitely open to understanding use-cases better. Are you hoping to take an arbitrary b1 and feed it into the add? Or is this part of a lowering of some other wider arithmetic operation? If the latter, is there any other instruction that would help?

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:16):

cfallin edited a comment on Issue #2860:

Greetings @Hypersonic and thanks for the report!

The plan we made when starting to design the new backend was to phase out add+carry (and sub+borrow) opcodes in the CLIF, instead directly providing the wider adds/subs that one would normally lower into carry-using ops. In the legalization-based old backend, i128.add became an i64.add and an i64.add_carry; now we just open-code the two-add sequence. (This was in discussions between myself, @julian-seward1 and @bnjbvr back in Jan 2020 or so.)

The reason for this design choice was that representing carry flags as normal values is fragile performance-wise: to get the correct (fast) machine code, which implicitly passes the bool in the carry flag, one has to pattern-match on the two instructions. The case where the b1 is separate -- as in your example, coming from a constant -- has to be lowered separately with pushf or setc to extract the flag and popf or a conditional diamond with clc/stc to set it. The materialized-carry-flag-value codepaths would almost never be used and so are a correctness risk as well; and these instructions are slow, to the point that there should (hopefully!) almost always be a better way to solve the problem.

Given that we just recently switched the default backend and haven't started the work of removing the old one, we haven't yet cleaned up the opcode space, but we intend to do so eventually.

That does leave the question: what if one actually wants to generate an add-with-carry? I'm definitely open to understanding use-cases better. Are you hoping to take an arbitrary b1 and feed it into the add? Or is this part of a lowering of some other wider arithmetic operation? If the latter, is there any other instruction that would help?

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:21):

cfallin edited a comment on Issue #2860:

Greetings @Hypersonic and thanks for the report!

The plan we made when starting to design the new backend was to phase out add+carry (and sub+borrow) opcodes in the CLIF, instead directly providing the wider adds/subs that one would normally lower into carry-using ops. In the legalization-based old backend, i128.add became an i64.add_ifcout and an i64.add_carry; now we just open-code the two-add sequence. (This was in discussions between myself, @julian-seward1 and @bnjbvr back in Jan 2020 or so.)

The reason for this design choice was that representing carry flags as normal values is fragile performance-wise: to get the correct (fast) machine code, which implicitly passes the bool in the carry flag, one has to pattern-match on the two instructions. The case where the b1 is separate -- as in your example, coming from a constant -- has to be lowered separately with pushf or setc to extract the flag and popf or a conditional diamond with clc/stc to set it. The materialized-carry-flag-value codepaths would almost never be used and so are a correctness risk as well; and these instructions are slow, to the point that there should (hopefully!) almost always be a better way to solve the problem.

Given that we just recently switched the default backend and haven't started the work of removing the old one, we haven't yet cleaned up the opcode space, but we intend to do so eventually.

That does leave the question: what if one actually wants to generate an add-with-carry? I'm definitely open to understanding use-cases better. Are you hoping to take an arbitrary b1 and feed it into the add? Or is this part of a lowering of some other wider arithmetic operation? If the latter, is there any other instruction that would help?

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:26):

bjorn3 commented on Issue #2860:

One use it to check for overflow. In cg_clif I currently have to manually check if an overflow has happened even though at least on x86 the processor already provides this information in the form of a flag.

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:27):

bjorn3 edited a comment on Issue #2860:

One use it to check for overflow to crash. In cg_clif I currently have to manually check if an overflow has happened even though at least on x86 the processor already provides this information in the form of a flag.

Wasmtime GitHub notifications bot (Apr 27 2021 at 17:36):

cfallin commented on Issue #2860:

@bjorn3 there is the iadd_ifcout instruction which is supported on the new backends (because stack-limit checks use it too IIRC) -- that should be usable to detect overflows? If not, we can add a variant that works for your case. Getting the flag out is easier (SETcc); it's the reloading into carry and use by subsequent operation, or pattern-matching to avoid that, that's slow/fragile.

Wasmtime GitHub notifications bot (Apr 27 2021 at 18:07):

Hypersonic commented on Issue #2860:

My main use case is detecting the carry-out -- I don't really have any particular need for the carry-in, but this was the instruction I found that would give me the output I needed. If there's a different instruction that can give me that, I'm more than happy to use it.

Wasmtime GitHub notifications bot (Apr 27 2021 at 19:12):

cfallin commented on Issue #2860:

@Hypersonic the iadd_ifcout instruction should give you just that, then, but if your case needs anything other then a trap, we'd need to add pattern-matching for e.g. selectif on its iflags result (right now we just match a trapif on its flags). We can certainly do that! I'm swamped at the moment but can get to this in a while if no one else is able to sooner.

Wasmtime GitHub notifications bot (Apr 27 2021 at 23:05):

Hypersonic commented on Issue #2860:

I think I'm covered by iadd_ifcout, thanks for the info! I'm going to close this issue since I think it's resolved for me.

Wasmtime GitHub notifications bot (Apr 27 2021 at 23:05):

Hypersonic closed Issue #2860:

Hi! While using Cranelift as a backend for a project, I ran into a panic compiling iadd_carry instructions on x86_64. Please let me know if there is any other information I can provide that would be helpful.

.clif Test Case

function u0:0() fast {
block0:
    v1465 = iconst.i32 0
    v1467 = bconst.b1 false
    v1468 = iconst.i32 0
    v1469, v1470 = iadd_carry v1465, v1468, v1467
    v1476 = bint.i8 v1470
    v1356 = iconst.i64 0
    v1391 -> v1356
    v1423 -> v1356
    v1447 -> v1356
    v1464 -> v1356
    v1494 -> v1356
    v1516 -> v1356
    v1544 -> v1356
    store notrap aligned v1476, v1356+274
    trap user0
}

Steps to Reproduce

cargo run -p cranelift-tools -- compile test.clif --target x86_64

Expected Results

Code successfully generates without a panic.

Actual Results

Panic: thread 'main' panicked at 'ALU+imm and ALU+carry ops should not appear here!', cranelift/codegen/src/isa/x64/lower.rs:5740:13

Versions and Environment

Cranelift version or commit: main HEAD (4a830b at time of writing)

Operating system: Linux

Architecture: x86_64

Last updated: Nov 22 2024 at 16:03 UTC