asm: fix sign-extended immediates #10216
Conversation
abrown
requested review from
fitzgen and
alexcrichton
and removed request for
a team and
fitzgen
As mentioned in bytecodealliance#10200, `capstone` has a peculiar way of pretty-printing immediates, especially signed immediates. It is simpler (and perhaps more clear) for us to just print immediates in one consistent format: `0xffff...`, e.g. This change parses capstone's pretty-printed immediates and converts them to our simpler format if the first attempt to match this assembler's output with `capston` fails.
As pointed out in bytecodealliance#10200, it could be confusing for users for `cranelift-assembler-x64` to pass unsigned integers to certain assembler instructions and have them unexpectedly sign-extended. Well, it can't be too surprising since these instructions have a `_sx*` suffix, but this change implements @alexcrichton's additional suggestion to create separate types for the immediates that may be sign-extended. These new types (`Simm8`, `Simm16`, `Simm32`) are quite similar to their vanilla counterparts (`Imm8`, `Imm16`, `Imm32`) but have additional sign-extension logic when pretty-printed. This means the vanilla versions can be simplified and the pre-existing `Simm32` is renamed to the more appropriate `AmodeOffset`.
abrown
force-pushed
the
assembler-simm
branch
from
67a3d20 to
c895162
Compare
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
This might be a bit of a silly question, but why not use uNN for AssemblerImmNN and iNN for AssemblerSimmNN? Do we benefit much from the differently-named versions with different methods/conversions?
Otherwise it sort of makes sense to me that instructions operate over logical values and they're represented with various bit-widths where uNN is always zero-extended to the operation width and iNN is sign-extended to the operation width (which doesn't change the logical meaning of the value, just extends the bit representation)
| @@ -24,6 +24,7 @@ pedantic = "warn" | |||
| module_name_repetitions = { level = "allow", priority = 1 } | |||
| similar_names = { level = "allow", priority = 1 } | |||
| wildcard_imports = { level = "allow", priority = 1 } | |||
| too_many_lines = { level = "allow", priority = 1 } | |||
There was a problem hiding this comment.
Would you be ok doing:
[lints]
workspace = truelike other crates in the workspace? That way we can ensure that lints are applied uniformly to all crates invovled. If you want a specific clippy lint for this crate then it can be done with #![warn(clippy::...)] in the crate itself
There was a problem hiding this comment.
Yeah, let me do that in another PR because it's going to be 25 changes unrelated to this one.
| fn is_imm32(&mut self, src: &GprMemImm) -> Option<AssemblerImm32> { | ||
| match src.clone().to_reg_mem_imm() { | ||
| RegMemImm::Imm { simm32 } => Some(AssemblerImm32::new(simm32)), | ||
| _ => None, | ||
| } | ||
| } | ||
|
|
||
| fn is_simm32(&mut self, src: &GprMemImm) -> Option<AssemblerSimm32> { | ||
| match src.clone().to_reg_mem_imm() { | ||
| RegMemImm::Imm { simm32 } => Some(AssemblerSimm32::new(simm32 as i32)), |
There was a problem hiding this comment.
Is as i32 necessary here? I thought simm32 was already i32?
|
@alexcrichton, I'm going to merge this to move things forward and open up some other PRs as follow-ups.
Yeah, it may have made more sense at some point but now the extra |
This removes all the custom clippy allowances and just adopts the project's Clippy settings. This was suggested over in [#10216]. [#10216]: #10216 (comment)
* asm: switch to using workspace clippy settings This removes all the custom clippy allowances and just adopts the project's Clippy settings. This was suggested over in [#10216]. [#10216]: #10216 (comment) * fix: add one more allow reason
#10200 described two main problems with sign-extended immediates in
cranelift-assembler-x64: (a) thecapstonepretty-printing had many peculiarities and (b) there is a potential for sign confusion when using sign-extending instructions. This PR solves (a) by parsing and re-printingcapstoneimmediates into a very vanilla hex format:$0xff.... It solves (b) by adding new types (Simm{8|16|32}) for sign-extending instructions. Each commit message has more details.This resolves #10200.