timjrd edited PR #6850:
Implements the following checks:
iconst.i8
immediate must be within 0 .. 0xFF
iconst.i16
immediate must be within 0 .. 0xFFFF
iconst.i32
immediate must be within 0 .. 0xFFFFFFFFfix #3059
Explain why this change is needed:
As mentioned in #3059,
iconst
currently allows any immediate within the range of ani64
, even foriconst.i8
,iconst.i16
oriconst.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.
timjrd updated PR #6850.
jameysharp submitted PR review:
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 submitted PR review:
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 created PR review comment:
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 submitted PR review.
timjrd created PR review comment:
OK! :smile: 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.
timjrd updated PR #6850.
timjrd edited PR #6850:
Add the following checks:
iconst.i8
immediate must be within 0 .. 2^8-1iconst.i16
immediate must be within 0 .. 2^16-1iconst.i32
immediate must be within 0 .. 2^32-1Resolves #3059
Explain why this change is needed:
As mentioned in #3059,
iconst
currently allows any immediate within the range of ani64
, even foriconst.i8
,iconst.i16
oriconst.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.
timjrd updated PR #6850.
timjrd requested jameysharp for a review on PR #6850.
timjrd submitted PR review.
timjrd created PR review comment:
@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 submitted PR review.
jameysharp created PR review comment:
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 submitted PR review.
timjrd created PR review comment:
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 submitted PR review.
jameysharp created PR review comment:
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.
timjrd updated PR #6850.
jameysharp submitted PR review:
Looks great to me! Let's see if this all passes CI and go ahead and merge it if so. Nice work!
timjrd submitted PR review.
timjrd created PR review comment:
The instructions are different. Is this expected?
timjrd created PR review comment:
Same as above.
timjrd submitted PR review.
timjrd has marked PR #6850 as ready for review.
timjrd requested wasmtime-compiler-reviewers for a review on PR #6850.
jameysharp merged PR #6850.
Last updated: Dec 23 2024 at 12:05 UTC