Wasmtime GitHub notifications bot (Jul 30 2024 at 22:25): Wasmtime GitHub notifications bot (Jul 30 2024 at 22:25):fitzgen requested wasmtime-compiler-reviewers for a review on PR #9046.
Wasmtime GitHub notifications bot (Jul 30 2024 at 22:25):fitzgen requested abrown for a review on PR #9046.
Wasmtime GitHub notifications bot (Jul 30 2024 at 22:25):fitzgen opened PR #9046 from fitzgen:dont-sign-extend-in-iconst to bytecodealliance:main:
Originally, we sign-extended
Imm64immediates from their controlling type var's width toi64in the builder methods forBinaryImm64and a couple other instruction formats: https://github.com/bytecodealliance/wasmtime/pull/1687At some point, we decided that the unused bits in the
Imm64when the controlling type var's width is less than 64 should be zero. And we started asserting that in various places, for example:
https://github.com/bytecodealliance/wasmtime/blob/87817f38a128caa76eaa6a3c3c8ceac81a329a3e/cranelift/codegen/src/machinst/lower.rs#L1237-L1243However, we never removed or updated the sign-extension code in the builder methods. This commit switches the builder methods over to masking the
Imm64to just the valid bits, effectively doing a zero extend from the controlling type var's width instead of a sign extend.To avoid too much churn in tests, I made
display_instprint the sign-extended-from-the-controlling-type-variable's-width value ofImm64immediates. I also made theDisplayimplementation forImm64always print in decimal form with a-for negative numbers, rather than the hex it would previously print for large negative numbers, so thaticonst.i32 0x8000_0000doesn't display asiconst.i32 0xffff_ffff_8000_0000, which is otherwise pretty confusing.<!--
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 (Jul 30 2024 at 22:49):cfallin submitted PR review:
Thanks for finding this inconsistency!
A few minor nits below; my only other thought is that the rules around how we print seem a little convoluted, though I understand the goal in trying to preserve existing test expectations. The asymmetry (small positive numbers, or any negative number, are decimal) somehow doesn't sit right with me. I think I'd almost rather take a one-time diff and pick something more consistent -- always hex? Always decimal? Always decimal but with hex in a comment (like we do for constants now e.g.
; v0 = 0)?Feel free to disregard my bikeshed color preferences; just my thoughts as long as we're here :-)
Wasmtime GitHub notifications bot (Jul 30 2024 at 22:49):cfallin submitted PR review:
Thanks for finding this inconsistency!
A few minor nits below; my only other thought is that the rules around how we print seem a little convoluted, though I understand the goal in trying to preserve existing test expectations. The asymmetry (small positive numbers, or any negative number, are decimal) somehow doesn't sit right with me. I think I'd almost rather take a one-time diff and pick something more consistent -- always hex? Always decimal? Always decimal but with hex in a comment (like we do for constants now e.g.
; v0 = 0)?Feel free to disregard my bikeshed color preferences; just my thoughts as long as we're here :-)
Wasmtime GitHub notifications bot (Jul 30 2024 at 22:49):cfallin created PR review comment:
To fit with the other operator methods (e.g.
wrapping_neg), can we do functional style here (fn mask_to_width(self, bit_width: u32) -> Self)?
Wasmtime GitHub notifications bot (Jul 30 2024 at 22:49):cfallin created PR review comment:
Can we rename the boolean on this if-block (
imms_need_sign_extension) as well to something that fits the new scheme?
Wasmtime GitHub notifications bot (Jul 30 2024 at 23:15):fitzgen updated PR #9046.
Wasmtime GitHub notifications bot (Jul 30 2024 at 23:16):fitzgen commented on PR #9046:
The asymmetry (small positive numbers, or any negative number, are decimal) somehow doesn't sit right with me. I think I'd almost rather take a one-time diff and pick something more consistent -- always hex? Always decimal? Always decimal but with hex in a comment (like we do for constants now e.g.
; v0 = 0)?Agreed. I'll look into this as a follow up, rather than doing it in this PR.
Wasmtime GitHub notifications bot (Jul 30 2024 at 23:16):fitzgen has enabled auto merge for PR #9046.
Wasmtime GitHub notifications bot (Jul 30 2024 at 23:56):fitzgen requested pchickey for a review on PR #9046.
Wasmtime GitHub notifications bot (Jul 30 2024 at 23:56):fitzgen updated PR #9046.
Wasmtime GitHub notifications bot (Jul 30 2024 at 23:56):fitzgen requested wasmtime-core-reviewers for a review on PR #9046.
Wasmtime GitHub notifications bot (Jul 31 2024 at 00:20):fitzgen merged PR #9046.
Last updated: Nov 22 2024 at 16:03 UTC