fitzgen requested wasmtime-compiler-reviewers for a review on PR #9046.
fitzgen requested abrown for a review on PR #9046.
fitzgen opened PR #9046 from fitzgen:dont-sign-extend-in-iconst
to bytecodealliance:main
:
Originally, we sign-extended
Imm64
immediates from their controlling type var's width toi64
in the builder methods forBinaryImm64
and a couple other instruction formats: https://github.com/bytecodealliance/wasmtime/pull/1687At some point, we decided that the unused bits in the
Imm64
when 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
Imm64
to 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_inst
print the sign-extended-from-the-controlling-type-variable's-width value ofImm64
immediates. I also made theDisplay
implementation forImm64
always 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_0000
doesn'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
-->
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 :-)
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 :-)
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
)?
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?
fitzgen updated PR #9046.
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.
fitzgen has enabled auto merge for PR #9046.
fitzgen requested pchickey for a review on PR #9046.
fitzgen updated PR #9046.
fitzgen requested wasmtime-core-reviewers for a review on PR #9046.
fitzgen merged PR #9046.
Last updated: Dec 23 2024 at 12:05 UTC