aarch64: Fix incorrect encoding of large const values in icmp. #3056
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bjorn3
reviewed
Jul 3, 2021
0c2bad1 to
236c3c1
Compare
cfallin
reviewed
Jul 3, 2021
Member
|
Ah, actually, nevermind the above; we don't have signedness at the CLIF type level (I need more coffee this morning) so this is a valid input and it's just an encoding issue. |
cfallin
approved these changes
Jul 3, 2021
Member
cfallin
left a comment
There was a problem hiding this comment.
OK, sorry about my misunderstanding above: this looks good!
Happy to merge with the mentioned simplification of the is_negative logic.
236c3c1 to
a5cc0b8
Compare
When encoding constants as immediates into an RSE Imm12 instruction we need to take special care to check if the value that we are trying to input does not overflow its type when viewed as a signed value. (i.e. iconst.i8 200) We cannot both put an immediate and sign extend it, so we need to lower it into a separate reg, and emit the sign extend into the instruction. For more details see the [cg_clif bug report](https://github.com/bjorn3/rustc_codegen_cranelift/issues/1184#issuecomment-873214796).
a5cc0b8 to
eebae8d
Compare
uweigand
added a commit
to uweigand/wasmtime
that referenced
this pull request
Sep 11, 2021
- Add relocation handling needed after PR bytecodealliance#3275 - Fix incorrect handling of signed constants detected by PR bytecodealliance#3056 test - Disable fuzzing tests that require pre-built v8 binaries - Disable cranelift test that depends on i128 - Temporarily disable memory64 tests
Merged
uweigand
added a commit
to uweigand/wasmtime
that referenced
this pull request
Sep 13, 2021
- Add relocation handling needed after PR bytecodealliance#3275 - Fix incorrect handling of signed constants detected by PR bytecodealliance#3056 test - Fix LabelUse max pos/neg ranges; fix overflow in buffers.rs - Disable fuzzing tests that require pre-built v8 binaries - Disable cranelift test that depends on i128 - Temporarily disable memory64 tests
alexcrichton
pushed a commit
that referenced
this pull request
Sep 20, 2021
- Add relocation handling needed after PR #3275 - Fix incorrect handling of signed constants detected by PR #3056 test - Fix LabelUse max pos/neg ranges; fix overflow in buffers.rs - Disable fuzzing tests that require pre-built v8 binaries - Disable cranelift test that depends on i128 - Temporarily disable memory64 tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When encoding constants as immediates into an RSE Imm12 instruction we need to take special care to check if the value that we are trying to input does not overflow its type when viewed as a signed value. (i.e. iconst.i8 200)
We cannot both put an immediate and sign extend it, so we need to lower it into a separate reg, and emit the sign extend into the instruction.
For more details see the cg_clif bug report.