Wasmtime GitHub notifications bot (Jun 05 2025 at 01:40): Wasmtime GitHub notifications bot (Jun 05 2025 at 01:40):jlb6740 opened PR #10928 from jlb6740:add-pcmp-to-new-assembler to bytecodealliance:main:
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:40):jlb6740 requested abrown for a review on PR #10928.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:40):jlb6740 requested wasmtime-compiler-reviewers for a review on PR #10928.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:43):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:43):jlb6740 created PR review comment:
This filetest update bothers me as the original result that includes "const(0)" is not valid syntax so I am not sure I can directly map this previous result to the current change and assume it is correct.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:43):jlb6740 edited PR review comment.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:44):jlb6740 edited PR review comment.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:44):jlb6740 deleted PR review comment.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:45):jlb6740 created PR review comment:
@abrown This filetest update bothers me since the original result that includes "const(0)" is not x86 syntax. Because it is not x86 syntax I am not sure how I can directly map this previous result to the current change to assume the blessed update is correct.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:45):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:45):jlb6740 deleted PR review comment.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:46):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:46):jlb6740 created PR review comment:
@abrown This filetest update bothers me since the original result that includes "const(0)" is not x86 syntax. Because it is not x86 syntax I am not sure how I can directly map this previous result to the current change to assume the blessed update is correct.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:46):jlb6740 deleted PR review comment.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:47):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:47):jlb6740 created PR review comment:
@abrown This filetest update bothers me since the original result includes code (const(0)) that is not valid x86 syntax. Because of this, I am not sure how I can directly map this previous result to the current update to assume this blessed update is correct.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:53):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 01:53):cfallin created PR review comment:
The previous syntax was representing a relocation that we perform after emitting, to refer to the constant pool. I don't know that we have to use the same syntax, but perhaps we could come up with a way to print the same information around the RIP-relative amode rather than just
(%rip)?
Wasmtime GitHub notifications bot (Jun 05 2025 at 20:37):jlb6740 created PR review comment:
@cfallin, I see and thanks for the education. I looked into this and talked with @abrown and it looks like we don't have information at the time of display that would alert us to tweak the output here to read
const(0)or anything else we choose. Perhaps we could figure out how to propagate that along but I am assuming the benefit would just be for highlighting a relocation reference during some stage of debugging. Do you think that is needed as a future todo? In any case for me, now that you've explained, it seems that task is unrelated to the correctness of pcmp* here, but are you also comfortable that I've bless the filetest solution update here in this pr?
Wasmtime GitHub notifications bot (Jun 05 2025 at 20:37):jlb6740 submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 21:09):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 21:09):cfallin created PR review comment:
Sure, this should be well-covered by runtests so if it's green, it's good to ship. I do suspect one could do something with traits here -- we already need to carry the amode with relocations somewhere, so perhaps we could have a "pretty-print" hook that Cranelift plugs into the assembler. No need to do that in this PR though, I don't think.
Wasmtime GitHub notifications bot (Jun 05 2025 at 22:37):abrown submitted PR review.
Wasmtime GitHub notifications bot (Jun 05 2025 at 23:15):abrown merged PR #10928.
Last updated: Dec 06 2025 at 07:03 UTC