x64: Fix panic compiling 16-bit multiply-with-immediate #10817
Conversation
This commit fixes a minor regression from bytecodealliance#10782 found via fuzzing. The regression is 16-bit immediates were forced to fit from an `i32` value into a `u16` for 16-bit multiplication. This meant though that negative numbers failed this conversion which meant that ISLE would panic due to the value not being matched. This fixes the logic to first fit the i32 into an i16 and then cast that to a u16 where the first phase should hit all the constants that are possible in Cranelift.
alexcrichton
requested review from
cfallin and
dicej
and removed request for
a team
| (rule 1 (x64_imul_imm $I16 src1 (u16_try_from_i32 src2)) (x64_imulw_rmi src1 src2)) | ||
| (rule 1 (x64_imul_imm $I32 src1 (i32_as_u32 src2)) (x64_imull_rmi src1 src2)) | ||
| (rule 1 (x64_imul_imm $I64 src1 src2) (x64_imulq_rmi_sxl src1 src2)) | ||
| (rule 1 (x64_imul_imm $I16 src1 (i16_try_from_i32 src2)) (x64_imulw_rmi src1 (i16_as_u16 src2))) |
There was a problem hiding this comment.
I'll admit this is testing the limits of my CLIF knowledge. This will fail when the i32 value fits in a u16, but not an i16, meaning that we'll still panic during instruction selection from that. I don't know whether that's possible in CLIF though, or if iconst values are always sign-extended from their type.
There was a problem hiding this comment.
Oh, missed that, sorry -- unfortunately we've standardized on zero extension instead (it keeps the majority of cases simpler). Fortunately if this extractor returns None then the rule just doesn't match -- no runtime panic unless there's not another fallback? Let's (i) add a test case with a value in that range (say 40000) and (ii) verify there is...
There was a problem hiding this comment.
Ok I've added a few CLIF tests for this and it looks like things are actually working as intended. Looks like when accessing the constant in ISLE it gets sign-extended based on the type of the constant itself, which is why this ended up working out. Yay!
2 participants
This commit fixes a minor regression from #10782 found via fuzzing. The regression is 16-bit immediates were forced to fit from an
i32value into au16for 16-bit multiplication. This meant though that negative numbers failed this conversion which meant that ISLE would panic due to the value not being matched. This fixes the logic to first fit the i32 into an i16 and then cast that to a u16 where the first phase should hit all the constants that are possible in Cranelift.