Wasmtime GitHub notifications bot (Jun 02 2025 at 18:57): Wasmtime GitHub notifications bot (Jun 02 2025 at 18:57):abrown opened PR #10899 from abrown:asm-pmovx to bytecodealliance:main:
This change converts all
pmovsx*andpmovzx*instructions to use the new assembler.
Wasmtime GitHub notifications bot (Jun 02 2025 at 18:57):abrown requested cfallin for a review on PR #10899.
Wasmtime GitHub notifications bot (Jun 02 2025 at 18:57):abrown requested wasmtime-compiler-reviewers for a review on PR #10899.
Wasmtime GitHub notifications bot (Jun 02 2025 at 18:58):abrown commented on PR #10899:
We can wait on merging this until #10881 is resolved to avoid merge conflicts for @jlb6740.
Wasmtime GitHub notifications bot (Jun 02 2025 at 19:34):cfallin submitted PR review:
Looks good overall, just a doc-comment request below; thanks!
Wasmtime GitHub notifications bot (Jun 02 2025 at 19:34):cfallin created PR review comment:
I see these are pre-existing (this part of the diff is just code motion) but even so, I'm not sure I understand what these variants mean -- could we have doc-comments on
xmm1,xmm2,xmm3describing what they're for and how they differ?
Wasmtime GitHub notifications bot (Jun 03 2025 at 03:35):jlb6740 created PR review comment:
@cfallin When describing an instruction a unique location is required for each reg/mem operand to avoid name clashes when generating lowering code (assembler.rs). Before vex, there was just a single xmm location and that was fine, but when describing vex instructions that use multiple xmm registers, that not longer was adequate. There was an attempt append subscripts to locations while generating code in assembler.rs, but it became clear that it made the most sense to create multiple xmm registers in the enum and just use the same subscript naming as is used as the Intel manual.
Wasmtime GitHub notifications bot (Jun 03 2025 at 03:35):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 03:41):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 03:41):cfallin created PR review comment:
OK, I'm not sure I understand why we need to define new types of operands though -- can't we have DSL methods that somehow specify which slot the registers correspond to?
In essence this seems like a category error to me: the enum so far defines the kinds of things we can operate on; an XMM register, a reg-or-mem, that sort of thing. Whether we use that as input X or Y, it's still that kind of thing. The fact that we use the operand in a certain way should be a property of the instruction, not the operand kind. The concern I have with this kind of conflation is that it's not at all clear to me (or presumably others in the future) which of these variants to use or what they mean -- a recipe for subtle bugs and footguns in the future. So I'd rather we find a way to store the information elsewhere...
Wasmtime GitHub notifications bot (Jun 03 2025 at 03:52):jlb6740 edited PR review comment.
Wasmtime GitHub notifications bot (Jun 03 2025 at 03:56):jlb6740 commented on PR #10899:
We can wait on merging this until #10881 is resolved to avoid merge conflicts for @jlb6740.
@abrown Thanks. #10881 should be resolved now.
Wasmtime GitHub notifications bot (Jun 03 2025 at 19:18):abrown submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 19:18):abrown created PR review comment:
Here's what is going on: this
enum Locationis actually the _concrete_ list of names accepted as operands, not a classification of operands. There are three reasons why this became concrete: (1) it makes it really simple for us to writeraxorm16orr32bwhen defining instructions, (2) that name then matches how the manual defines the instruction, and (3) that name is then unique for the operand and we can use it as Rust field, e.g.When it comes to _classifications_ of operands, we actually have two different ways of categorizing operands:
enum OperandKind, which we use to distinguish betweenReg,Mem, etc.enum RegClass, which we use to distinguish betweenXmmandGprThe concrete
Locationnames get categorized appropriately inLocation::kind()andLocation::reg_class(). Most of the assembler code generation uses their output. I think it would be helpful to summarize all of this this text above in the doc comments ofLocation, which is what @cfallin you were originally asking, right? I think that would be a good addition: do you have any specific wording choices that would be most helpful here?
Wasmtime GitHub notifications bot (Jun 04 2025 at 15:50):abrown updated PR #10899.
Wasmtime GitHub notifications bot (Jun 04 2025 at 15:54):abrown updated PR #10899.
Wasmtime GitHub notifications bot (Jun 04 2025 at 17:19):abrown submitted PR review.
Wasmtime GitHub notifications bot (Jun 04 2025 at 17:19):abrown created PR review comment:
For later reference: we discussed this in the Cranelift meeting today and @cfallin is planning to look into this a bit more. There are other ways to factor this that avoids having the concrete things in the same bucket with general ones while still "looking like the manual" and having unique Rust field names; @alexcrichton had some thoughts to write up in an issue. I'll go ahead and merge this for now since this discussion was unrelated to this PR.
Wasmtime GitHub notifications bot (Jun 04 2025 at 17:21):abrown submitted PR review.
Wasmtime GitHub notifications bot (Jun 04 2025 at 17:21):abrown created PR review comment:
See https://github.com/bytecodealliance/wasmtime/issues/10922 for future discussion.
Wasmtime GitHub notifications bot (Jun 04 2025 at 17:42):abrown merged PR #10899.
Last updated: Dec 06 2025 at 06:05 UTC