cranelift: Validate iconst ranges

[timjrd]

Add the following checks:

• iconst.i8 immediate must be within 0 .. 2^8-1
• iconst.i16 immediate must be within 0 .. 2^16-1
• iconst.i32 immediate must be within 0 .. 2^32-1

Resolves #3059

Explain why this change is needed:

As mentioned in #3059, iconst currently allows any immediate within the range of an i64, even for iconst.i8, iconst.i16 or iconst.i32.

This breaks some tests!

Running cargo test in /cranelift/codegen returns successfully. I also added a few tests concerning the new checks.

Running cargo test in /cranelift returns some failures. For example:

1: - inst1 (v1 = iconst.i8 -1): constant immediate is out of bounds
[...]
failures:
    bugpoint::tests::test_reduce
    run::test::nop

Which is expected if we forbid negative immediates, @jameysharp could you please confirm?

Running cargo test at the root of this repo returns a lot of failures, possibly related to negative immediates.

This is my first contribution so this patch is probably broken.

[jameysharp]

I think it's a good sign that the verifier is rejecting those tests. It's nice to confirm that it's detecting the problem that it was supposed to check for!

That said, I think the way we should fix these tests is by changing the parser for the textual CLIF representation, in cranelift/reader/src/parser.rs.

The first thing to note is that I believe parse_inst_operands should only be called for an InstructionFormat::UnaryImm if explicit_control_type is not None; I think iconst requires that a type always be provided, and it's the only instruction which uses that instruction format.

So when parsing operands for that format, we can report a syntax error if that argument is None, and otherwise match on the type to decide whether to call self.match_imm8/16/32. Those return signed types like i8, which needs to be converted to unsigned so it's zero-extended. So something like Imm64::from(self.match_imm8(...) as u8) should check that it fits in range for the appropriate type.

Does that make sense? Feel free to do that in this PR, or make a separate PR for that change which we'll merge first, either way.

[timjrd]

After looking at the CLIF parser, it does make total sense. Thanks for the detailed explanation. 🙂

I have a somewhat working patch, I need to fix a few things and it should be good!

[timjrd]

Hi @jameysharp 👋 please see the message of my last commit: I'm facing some test breakage, I fixed a few ones, but many others seem to be related to the conversion from signed to unsigned integers. Maybe I should adapt the ranges in the verifier to allow both signed and unsigned use (-128 .. 255 instead of 0 .. 255 for i8) as initially proposed by @afonso360? 🙂

[jameysharp]

I still don't think that we want to allow sign-extended values in iconst instructions, and I don't think we need to.

Most of the remaining test failures are in the precise-output tests for the riscv64 backend, so I hope @afonso360 has time to take a look. There is one similar failure in each of x64 and s390x, and none in aarch64.

In all the failing precise-output tests, I think the backend is emitting constants where the upper bits are subsequently ignored so it doesn't matter what value they have, and this has changed only those upper bits. We need to verify that though. And it looks like riscv64 is missing an opportunity to use shorter sequences of instructions when those high bits don't matter.

[jameysharp]

The handful of test failures on your branch that are in filetests/filetests/egraph/ are because of this change: constants which were narrower than 64 bits were previously printed as unsigned, and are now printed as signed, but these tests expect the printed text to match what's in the test.

This change is harmless and I kind of like having these values printed the way you've done it, so you can fix the egraph tests by updating them to match the new format.

[timjrd]

OK! 🙂 The change in write.rs was needed to fix a test case, but ended up breaking some others, as you said. I'll update those outputs.

[jameysharp]

One more thought: The fact that these backends produce different code based on the supposedly-ignored high bits of iconst instructions is potentially a sign of bugs in the backends. Hopefully these cases are all harmless but it's hard to tell at a glance. So these test failures are a good sign! They indicate that this PR will help us ensure that there aren't any surprising ways to affect codegen from a Cranelift frontend.

[afonso360]

Hey, Just wanted to say that this is awesome work! Thanks for working on it. I'm going to look into the RISC-V failures, but It might take a bit.

[bjorn3]

Would it make sense to store Uimm64 instead of Imm64 in iconst given that sign extending is no longer allowed?

[jameysharp]

Would it make sense to store Uimm64 instead of Imm64 in iconst given that sign extending is no longer allowed?

Yes, I think so—but I think that's a much bigger change and we shouldn't try to do it in this PR.

[timjrd]

Alright! I'm waiting for a greenlight (or a fix) before updating the outputs of the tests, since I'm a total ISAs / assembly / compiler noob.

After this PR is merged, I would be happy to work on replacing Imm64 by Uimm64, if the task is suitable for a compiler beginner with limited time resource. It's a related change, so at least I know where to start. 🙂

[jameysharp]

Now that #6915 is merged (thanks @afonso360!), hopefully most of the test failures will disappear in this PR. Are you comfortable rebasing on main and taking another look at which tests fail now?

[timjrd]

I updated the egraphs file-tests. There are 2 remaining failures:

FAIL filetests/filetests/isa/x64/simd-lane-access-compile.clif: compile
line 185

    --- expected
    +++ actual
    @@ -2,7 +2,7 @@
       pushq   %rbp
       movq    %rsp, %rbp
     block0:
    -  movl    $-1, %ecx
    +  movl    $65535, %ecx
       movd    %ecx, %xmm1
       pshuflw $0, %xmm1, %xmm3
       pshufd  $0, %xmm3, %xmm0
    @@ -15,7 +15,7 @@
       pushq %rbp
       movq %rsp, %rbp
     block1: ; offset 0x4
    -  movl $0xffffffff, %ecx
    +  movl $0xffff, %ecx
       movd %ecx, %xmm1
       pshuflw $0, %xmm1, %xmm3
       pshufd $0, %xmm3, %xmm0

FAIL filetests/filetests/isa/s390x/constants.clif: compile
line 4

    --- expected
    +++ actual
    @@ -1,9 +1,9 @@
     VCode:
     block0:
    -  lhi %r2, -1
    +  lhi %r2, 255
       br %r14
     
     Disassembled:
     block0: ; offset 0x0
    -  lhi %r2, -1
    +  lhi %r2, 0xff
       br %r14

[timjrd]

@jameysharp this is not super clean, the parser could not re-parse this output, I don't know if it's a big deal

[jameysharp]

Yeah, something's a little off about these tests, which appear to be sign-extending 32-bit constants to 64-bit when printing them. I think there's a bug in the writer but I haven't looked closely at it to figure out what that might be.

[timjrd]

I didn't check but most probably the Display instance for Imm64 is falling back to hex representation for big values. This particular example is a valid negative i32 value, but a big one, so maybe the Display implementation is falling back to unsigned 64-bit hexadecimal as it can't know it's a 32-bit value. We could add range checks in this Display instance and emit the right amount of hex digits; or I could remove my changes in the CLIF writer altogether and figure out another way of fixing the tests it solved.

[jameysharp]

I see now. Before this PR, for constants narrower than 64 bits we were preserving whether the input was written as a negative number or not based on whether the Imm64 was sign-extended or zero-extended. So some of the egraph tests used small negative numbers on input and got negative numbers in the test expectations output; others used large enough positive numbers to have the high bit set, which were printed as positive hex numbers before but get sign-extended by your change to the writer now.

I think we should revert your change to the writer so it doesn't try to sign-extend constants, and then we'll probably just need to update the test expectations to reflect that some negative constants are printed as large positive hex values now.

[jameysharp]

This feels very close!

@abrown and @uweigand, could you take a look at the test failures happening on this PR and check my reasoning about them? The diffs in the precise-output compile tests are in a comment above.

---

For s390x, the failing test is:

wasmtime/cranelift/filetests/filetests/isa/s390x/constants.clif

Lines 4 to 18 in 62fdafa

	function %f() -> i8 {
	block0:
	v0 = iconst.i8 -1
	return v0
	}
	; VCode:
	; block0:
	; lhi %r2, -1
	; br %r14
	;
	; Disassembled:
	; block0: ; offset 0x0
	; lhi %r2, -1
	; br %r14

I think the lowering rule in question is this one:

wasmtime/cranelift/codegen/src/isa/s390x/inst.isle

Lines 2985 to 2989 in 62fdafa

	;; 16-bit (or smaller) result type, any value
	(rule 7 (imm (fits_in_16 ty) n)
	(let ((dst WritableReg (temp_writable_reg ty))
	(_ Unit (emit (MInst.Mov32SImm16 dst (u64_as_i16 n)))))
	dst))

I'm guessing that callers are expected to ignore all but the least-significant 8 bits of %r2 if a function returns an I8, so it wouldn't matter that in this case the constant has been sign-extended past 8 bits. It also doesn't matter to Wasmtime, I think, since we don't use 8-bit constants and for anything wider there are type-specific lowering rules. But I think it's hard to prove that there aren't any other situations where a sign-extended 8-bit constant could cause other instructions to produce incorrect results.

That said, my guess is that we can just update this test expectation from "-1" to "255"/"0xff" and everything else will be good enough as-is.

---

For x64, this is the failing test:

wasmtime/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif

Lines 185 to 215 in 62fdafa

	function %splat_i16() -> i16x8 {
	block0:
	v0 = iconst.i16 -1
	v1 = splat.i16x8 v0
	return v1
	}
	; VCode:
	; pushq %rbp
	; movq %rsp, %rbp
	; block0:
	; movl $-1, %ecx
	; movd %ecx, %xmm1
	; pshuflw $0, %xmm1, %xmm3
	; pshufd $0, %xmm3, %xmm0
	; movq %rbp, %rsp
	; popq %rbp
	; ret
	;
	; Disassembled:
	; block0: ; offset 0x0
	; pushq %rbp
	; movq %rsp, %rbp
	; block1: ; offset 0x4
	; movl $0xffffffff, %ecx
	; movd %ecx, %xmm1
	; pshuflw $0, %xmm1, %xmm3
	; pshufd $0, %xmm3, %xmm0
	; movq %rbp, %rsp
	; popq %rbp
	; retq

When the x64 backend lowers (splat (iconst x)) where x is an i16, it emits a 32-bit constant, which I think is because the matching lowering rule invokes bitcast_gpr_to_xmm $I32. As a result, it includes bits in the constant which are supposed to be ignored.

wasmtime/cranelift/codegen/src/isa/x64/lower.isle

Lines 4433 to 4434 in 62fdafa

	(rule 0 (lower (has_type $I16X8 (splat src)))
	(x64_pshufd (x64_pshuflw (bitcast_gpr_to_xmm $I32 src) 0) 0))

I believe this is harmless because the value is used by pshuflw which is told to read only the lowest 16 bits, overwriting the uninitialized bits. If so, we can safely update the test expectation in this PR because only unused bits change. But I wonder if we should change the generated code further for this case? For example, does movw $-1, %cx have a shorter encoding than movl $-1, %ecx, or weird costs? Would it be worth matching on splat of iconst.i16 and precomputing the result of pshuflw as a 32-bit constant, if we have to emit a 32-bit constant anyway? etc.

[uweigand]

I'm guessing that callers are expected to ignore all but the least-significant 8 bits of %r2 if a function returns an I8, so it wouldn't matter that in this case the constant has been sign-extended past 8 bits. It also doesn't matter to Wasmtime, I think, since we don't use 8-bit constants and for anything wider there are type-specific lowering rules. But I think it's hard to prove that there aren't any other situations where a sign-extended 8-bit constant could cause other instructions to produce incorrect results.

The general assumption in the s390x back-end is that when holding an i8 in a register, the upper bits are undefined and don't-care. (For example, i8 addition uses a 32-bit addition ISA instruction, which will change upper bits.) From that perspective, using lhi .., -1 and lhi .., 255 should be equivalent when loading an i8 value. (Also note that lhi only modifies the low 32 bits of the 64-bit register in the first place, the high bits are left unmodified. So either way this doesn't deterministically set all bits in the full register.)

As to the caller's expectations, the platform ABI does indeed require all scalar integer types to be zero- or sign-extended to the full register width when passed in registers. However, this is implemented at the cranelift IR level by using i8 uext or i8 sext as argument or return type, not a plain i8. (The latter is only used in rare cases like when passing a single-byte struct by value. In those cases the ABI does indeed state that the upper bits are undefined, so i8 correctly represents that semantics.)

That said, my guess is that we can just update this test expectation from "-1" to "255"/"0xff" and everything else will be good enough as-is.

Agreed.

[abrown]

I believe this is harmless because the value is used by pshuflw which is told to read only the lowest 16 bits, overwriting the uninitialized bits. If so, we can safely update the test expectation in this PR because only unused bits change. But I wonder if we should change the generated code further for this case? For example, does movw $-1, %cx have a shorter encoding than movl $-1, %ecx, or weird costs? Would it be worth matching on splat of iconst.i16 and precomputing the result of pshuflw as a 32-bit constant, if we have to emit a 32-bit constant anyway? etc.

@jameysharp:

• Your assumption is correct: pshuflw reads only the low 16 bits; you could just change the test
• I don't think you're going to get smaller code with a word-mov; it's the same single byte opcode as a doubleword (32 bits in x86)
• I like your idea of changing the codegen: I would just double up the 16-bit constant in the 32-bit mov and then use a single pshufd: for constant a, movl <a>:<a>, %ecx; movd %ecx, %xmm1; pshufd $0, %xmm1, %xmm1. (Another option would be to use a pshufb but then you have to setup a separate xmm register for the mask).

[jameysharp]

Awesome: besides sorting out the writer change that's affecting the egraph tests, the last step for this PR is to update these two test expectations to match what the new parser implementation does.

---

I don't think you're going to get smaller code with a word-mov; it's the same single byte opcode as a doubleword (32 bits in x86)

I see it's the same opcode, but the immediate operand is only two bytes instead of four, right? Looking more carefully, I'm confused by the use of operand_size_of_type_32_64 in the imm constructor used by iconst. Why doesn't it use smaller move-from-immediate instructions for smaller types? Is this just to ensure that the upper bits in the register are initialized, or perhaps because nothing else needed smaller operand sizes?

wasmtime/cranelift/codegen/src/isa/x64/inst.isle

Lines 2568 to 2572 in 62fdafa

	(rule 1 (imm (fits_in_64 ty) (u64_nonzero simm64))
	(let ((dst WritableGpr (temp_writable_gpr))
	(size OperandSize (operand_size_of_type_32_64 ty))
	(_ Unit (emit (MInst.Imm size simm64 dst))))
	dst))

[jameysharp]

Looks great to me! Let's see if this all passes CI and go ahead and merge it if so. Nice work!

[jameysharp]

Oh, though you still have it marked as a draft PR. Do you feel it's ready to merge? You should click the "Ready for review" button if so.

[timjrd]

The instructions are different. Is this expected?

[timjrd]

Same as above.

[timjrd]

@jameysharp I think it's OK except for the 2 comments I made above. 🙂

[jameysharp]

Those two diffs are the same s390x instruction printed by two different disassemblers, so it's actually only one change. I looked at it before approving the PR earlier and decided that, although I don't know what the iilf instruction does, it's clearly emitting a 32-bit immediate constant and the instruction it came from is an iconst.i32, so it's probably fine. It's possible that @uweigand may want to change codegen based on this, if lhi is a more efficient instruction encoding or something, but I think that can safely land after this PR if it's worth doing at all.

[timjrd]

Nice to see this PR merged! Thanks for all your detailed explanations and guidance. This project is definitely beginner-friendly! 🚀

[uweigand]

Those two diffs are the same s390x instruction printed by two different disassemblers, so it's actually only one change. I looked at it before approving the PR earlier and decided that, although I don't know what the iilf instruction does, it's clearly emitting a 32-bit immediate constant and the instruction it came from is an iconst.i32, so it's probably fine. It's possible that @uweigand may want to change codegen based on this, if lhi is a more efficient instruction encoding or something, but I think that can safely land after this PR if it's worth doing at all.

This is now #6952