Maja Kądziołka (Feb 24 2024 at 22:06): Maja Kądziołka (Feb 24 2024 at 22:06):I've been browsing the cranelift code and I noticed the following suspicious line:
If I'm reading this right, the opcode for adc r, r/m is wrong – the one for add r, r/m is actually getting used. I tried writing a testcase that triggers this, and iadd_carry causes an ICE because we don't have a lowering rule for it at all. What's going on?
Chris Fallin (Feb 25 2024 at 05:52):@Maja Kądziołka I'm not sure where you're getting "wrong opcode" from -- the opcode_r here is 0x01 for ADD and 0x11 for ADC, which is correct (see the "Add with CF r64 to r/m64" case)
Chris Fallin (Feb 25 2024 at 05:53):iadd_carry is not supported and should probably either be removed or filled out; it's an artifact of an older legalization-based approach to "wide" (128-bit) ops. Currently operations on i128s will natively use adc/add pairs during lowering, so all of this is transparent at the CLIF level
Maja Kądziołka (Feb 25 2024 at 10:58):opcode_m isn't correct, though!
Chris Fallin (Feb 25 2024 at 17:29):@Maja Kądziołka ah, yes, that's right; we haven't hit this because we don't use the add-to-memory form of adc (or sbb) at all. If you'd like, I'm happy to review a PR fixing that and adding emit-tests to cover.
And to your second question and if you're feeling ambitious, removing iadd_carryand isub_borrow from Cranelift too; it looks like they only appear in the CLIF interpreter and in some interpreter-only runtests
Last updated: Nov 26 2024 at 00:12 UTC