Wasmtime GitHub notifications bot (May 27 2025 at 02:47): Wasmtime GitHub notifications bot (May 27 2025 at 02:47):rahulchaphalkar opened PR #10836 from rahulchaphalkar:compare-instructions-rebase to bytecodealliance:main:
Implement
compareinstructions.
ImplementEflagslogic for DSL and wire up isle rules to set flags when appropriate.
Wasmtime GitHub notifications bot (May 27 2025 at 02:47):rahulchaphalkar requested abrown for a review on PR #10836.
Wasmtime GitHub notifications bot (May 27 2025 at 02:47):rahulchaphalkar requested wasmtime-compiler-reviewers for a review on PR #10836.
Wasmtime GitHub notifications bot (May 27 2025 at 02:54):rahulchaphalkar updated PR #10836.
Wasmtime GitHub notifications bot (May 27 2025 at 03:53):github-actions[bot] commented on PR #10836:
Subscribe to Label Action
cc @saulecabrera
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "cranelift:meta", "winch"Thus the following users have been cc'd because of the following labels:
- saulecabrera: winch
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 (May 27 2025 at 15:47):rahulchaphalkar updated PR #10836.
Wasmtime GitHub notifications bot (May 27 2025 at 16:08):rahulchaphalkar updated PR #10836.
Wasmtime GitHub notifications bot (May 27 2025 at 16:10):rahulchaphalkar updated PR #10836.
Wasmtime GitHub notifications bot (May 28 2025 at 01:41):abrown assigned abrown to PR #10836.
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown submitted PR review.
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown created PR review comment:
I propose we just name this
Eflagsand then write documentation explaining why this is needed, what it models, and where this might head in the future (i.e., we might want to start modeling individual flag bits). It might also be good to link to https://github.com/bytecodealliance/wasmtime/issues/10298.
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown created PR review comment:
I'm not a big fan of this; the
lock_stuff below already seemed unfortunate but now this opens a whole new can of worms. There are a few other instructions that probably are in the same situation, notably jumps, so a better overall solution seems like a good idea.In a separate PR, we could:
- give
dsl::Insta way to specify an externalto_stringfunction- here we just say, "if an instruction has special pretty-printing, simply emit that function call"
- then we define that function up at the assembler level, where it's easier to inspect, test, etc.
Perhaps we merge this as-is and refactor later. But we could try the "custom printing" idea prior to this merging and rebase on it. I'm open to either, I just think we should think through a better long-term solution. Thoughts?
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown created PR review comment:
Do we end up using this?
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown created PR review comment:
We don't end up lowering to these instructions yet, right? If so, a simplification here could be to keep these instructions out of this PR and add them in once we can do the ISLE conversions at the same time. (I mean, we could also leave them in this PR since we know we'll eventually use them, I'm just flagging that they're not yet used).
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown created PR review comment:
Shouldn't we be setting
flagsfor all of these?
Wasmtime GitHub notifications bot (May 28 2025 at 18:07):abrown created PR review comment:
Like we were talking about in the Cranelift meeting, I think we want to reduce these methods hanging off of
Inst. We can probably just fit into the existingmatch sizeabove, but if that doesn't work, I also started using this pattern:
Wasmtime GitHub notifications bot (May 31 2025 at 01:40):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 02 2025 at 17:49):abrown created PR review comment:
Ok, let's leave this as-is for now but we'll need to refactor to something more like the
customlogic introduced by @alexcrichton in https://github.com/bytecodealliance/wasmtime/pull/10887.
Wasmtime GitHub notifications bot (Jun 02 2025 at 17:49):abrown submitted PR review:
I think this makes sense; the only remaining refactoring is to unwrap some unnecessary
Inst::*code and then this should be good-to-go.
Wasmtime GitHub notifications bot (Jun 02 2025 at 17:49):abrown created PR review comment:
How about this to match the others?
asm::inst::ucomiss_a::new(dst, lhs).into(),
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:18):rahulchaphalkar created PR review comment:
Hey, I agree with the idea that lets push the external printing patch first, and then rebase on that. So we can leave this PR open for a bit until I push the other PR, I'll take a look at at https://github.com/bytecodealliance/wasmtime/pull/10887
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:18):rahulchaphalkar submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:18):rahulchaphalkar submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:18):rahulchaphalkar created PR review comment:
Removed
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:19):rahulchaphalkar submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:19):rahulchaphalkar created PR review comment:
Yes, removed these instructions for now since they weren't lowered
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:20):rahulchaphalkar submitted PR review.
Wasmtime GitHub notifications bot (Jun 03 2025 at 16:20):rahulchaphalkar created PR review comment:
It needs type annotation, I don't know why only for these couple of instructions rust is unable to infer type, so was having issues with using Into even with types. I can retry with
intowith type annotations, but it still won't match exactly.
Wasmtime GitHub notifications bot (Jun 09 2025 at 19:18):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 09 2025 at 19:40):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 09 2025 at 19:41):rahulchaphalkar commented on PR #10836:
@abrown
I have rebased this on theCustomlogic PR as well as latest main.
For compare instructions, we need to change operands as well (remove the immediate) as well as change name, so now we are passingordered_opsstring tocustom::Displayfunctions, which returns a concatenated string containingnameandordered_ops.
Wasmtime GitHub notifications bot (Jun 09 2025 at 22:07):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 10 2025 at 16:24):abrown created PR review comment:
Do you need help with this part?
Wasmtime GitHub notifications bot (Jun 10 2025 at 16:24):abrown submitted PR review.
Wasmtime GitHub notifications bot (Jun 10 2025 at 16:24):abrown created PR review comment:
It seems like in the end we probably just want this to be:
pub fn cmppd_a<R: Registers>(f: &mut std::fmt::Formatter, inst: &inst::cmppd_a<R>) -> std::fmt::Result {The problem with that is that then we have to list out the operands of each instruction in order, so I guess we can go with this for now.
Wasmtime GitHub notifications bot (Jun 11 2025 at 16:07):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 11 2025 at 16:32):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 11 2025 at 16:33):rahulchaphalkar submitted PR review.
Wasmtime GitHub notifications bot (Jun 11 2025 at 16:33):rahulchaphalkar created PR review comment:
Done
Wasmtime GitHub notifications bot (Jun 11 2025 at 17:44):rahulchaphalkar created PR review comment:
Got it, will push those changes to a later PR.
Wasmtime GitHub notifications bot (Jun 11 2025 at 17:44):rahulchaphalkar submitted PR review.
Wasmtime GitHub notifications bot (Jun 11 2025 at 22:20):abrown submitted PR review:
LGTM! I think the one last thing we'll want to clean up the
ordered_opsbits once https://github.com/bytecodealliance/wasmtime/pull/11017 is merged.
Wasmtime GitHub notifications bot (Jun 11 2025 at 22:20):abrown submitted PR review:
LGTM! I think the one last thing we'll want is to clean up the
ordered_opsbits once https://github.com/bytecodealliance/wasmtime/pull/11017 is merged.
Wasmtime GitHub notifications bot (Jun 12 2025 at 19:17):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 12 2025 at 19:18):rahulchaphalkar commented on PR #10836:
@abrown ok i have rebased and pushed with changes, CI still running.
Wasmtime GitHub notifications bot (Jun 12 2025 at 19:44):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 12 2025 at 19:56):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 12 2025 at 20:02):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 12 2025 at 20:58):rahulchaphalkar updated PR #10836 (assigned to abrown).
Wasmtime GitHub notifications bot (Jun 12 2025 at 21:11):abrown has enabled auto merge for PR #10836.
Wasmtime GitHub notifications bot (Jun 12 2025 at 21:41):abrown merged PR #10836.
Last updated: Dec 06 2025 at 06:05 UTC