Wasmtime GitHub notifications bot (Feb 10 2025 at 23:17): Wasmtime GitHub notifications bot (Feb 10 2025 at 23:17):abrown opened PR #10216 from abrown:assembler-simm to bytecodealliance:main:
#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.
Wasmtime GitHub notifications bot (Feb 10 2025 at 23:17):abrown requested fitzgen for a review on PR #10216.
Wasmtime GitHub notifications bot (Feb 10 2025 at 23:17):abrown requested wasmtime-compiler-reviewers for a review on PR #10216.
Wasmtime GitHub notifications bot (Feb 10 2025 at 23:29):abrown updated PR #10216.
Wasmtime GitHub notifications bot (Feb 10 2025 at 23:29):abrown requested alexcrichton for a review on PR #10216.
Wasmtime GitHub notifications bot (Feb 10 2025 at 23:36):abrown updated PR #10216.
Wasmtime GitHub notifications bot (Feb 11 2025 at 01:04):github-actions[bot] commented on PR #10216:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
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:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Feb 12 2025 at 07:47):alexcrichton submitted PR review:
This might be a bit of a silly question, but why not use
uNNforAssemblerImmNNandiNNforAssemblerSimmNN? 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
uNNis always zero-extended to the operation width andiNNis sign-extended to the operation width (which doesn't change the logical meaning of the value, just extends the bit representation)
Wasmtime GitHub notifications bot (Feb 12 2025 at 07:47):alexcrichton created PR review 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
Wasmtime GitHub notifications bot (Feb 12 2025 at 07:47):alexcrichton created PR review comment:
Is
as i32necessary here? I thoughtsimm32was alreadyi32?
Wasmtime GitHub notifications bot (Feb 13 2025 at 22:14):abrown submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 22:14):abrown created PR review comment:
Yeah, let me do that in another PR because it's going to be 25 changes unrelated to this one.
Wasmtime GitHub notifications bot (Feb 13 2025 at 22:15):abrown submitted PR review.
Wasmtime GitHub notifications bot (Feb 13 2025 at 22:15):abrown created PR review comment:
It's weirdly
u32?
Wasmtime GitHub notifications bot (Feb 13 2025 at 22:17):abrown commented on PR #10216:
@alexcrichton, I'm going to merge this to move things forward and open up some other PRs as follow-ups.
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?
Yeah, it may have made more sense at some point but now the extra
Imm*::new(...)andImm*::value()functions don't make much sense. We can just have some functions that do the appropriate pretty-printing.
Wasmtime GitHub notifications bot (Feb 13 2025 at 22:38):abrown merged PR #10216.
Last updated: Feb 28 2025 at 01:30 UTC