Wasmtime GitHub notifications bot (Jan 21 2025 at 16:42): Wasmtime GitHub notifications bot (Jan 21 2025 at 16:42):alexcrichton opened PR #10062 from alexcrichton:pulley-imul-i128 to bytecodealliance:main:
While Pulley has lowering rules for widening multiplication it didn't have a rule for a full 128-bit multiplication which is possible to generate through CLIF optimizations given wasm input. This commit adds such a lowering to the Cranelift backend but doesn't add any new instructions yet under the assumption this probably isn't perf-critical at this time.
<!--
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 (Jan 21 2025 at 16:42):alexcrichton requested cfallin for a review on PR #10062.
Wasmtime GitHub notifications bot (Jan 21 2025 at 16:42):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #10062.
Wasmtime GitHub notifications bot (Jan 21 2025 at 16:57):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 21 2025 at 16:57):cfallin created PR review comment:
indented one space too far here? (sorry, minor issue, my Lisp sensibilities are just causing an eyeball twitch)
Wasmtime GitHub notifications bot (Jan 21 2025 at 16:57):cfallin created PR review comment:
Actually -- we're adding to a user-controlled initial offset -- should we have a generic fallback here?
Wasmtime GitHub notifications bot (Jan 21 2025 at 16:57):cfallin created PR review comment:
Perhaps a comment here noting that we control
offset(constants in our lowering rules) so we don't have to worry about the case where theif-letfails?
Wasmtime GitHub notifications bot (Jan 21 2025 at 16:57):cfallin created PR review comment:
likewise here
Wasmtime GitHub notifications bot (Jan 21 2025 at 20:29):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jan 21 2025 at 20:29):alexcrichton created PR review comment:
This is only used for i128 stores which is only used by Cranelift trampolines in tests, and technically it's universally correct just perhaps not as useful as it could be in the sense that it'll fail if the offset addition overflows in the future. Basically I wasn't sure if it was worth trying to be 100% correct here when nothing known will hit this, so I opted for a "will loudly fail" in the future rather than filling out all the particulars to get this working optimally. I'll look at it again though and see if I can't think of an always-works solution.
Wasmtime GitHub notifications bot (Jan 21 2025 at 20:32):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jan 21 2025 at 20:32):alexcrichton created PR review comment:
Currently ISLE backends differ a bit on style where the x64 backend likes to line things up by the previous line in ISLE (I think the style you're expecting here) but the riscv64 backend takes a different approach where the next line is just indented by one more than the previous line at all times (which is the style I was writing here). Personally I prefer the riscv64 style because I find it easier to work with editors and I don't have to think too hard about lining things up all the time.
That being said I don't really have a strong preference either way, but in the absence of a style guide/formatter I'll probably leave this as-is since it doesn't deviate too far from the current style (AFAIK)
Wasmtime GitHub notifications bot (Jan 21 2025 at 21:52):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jan 21 2025 at 21:52):cfallin created PR review comment:
If you prefer a different bikeshed color then don't take my comment too seriously -- in the fullness of time maybe we'll have an autoformatter (and then everything will be in the One True S-expr Indent Style) but no worries otherwise :-)
Wasmtime GitHub notifications bot (Jan 21 2025 at 22:15):alexcrichton updated PR #10062.
Wasmtime GitHub notifications bot (Jan 21 2025 at 22:16):alexcrichton has enabled auto merge for PR #10062.
Wasmtime GitHub notifications bot (Jan 21 2025 at 22:55):alexcrichton merged PR #10062.
Last updated: Jan 24 2025 at 00:11 UTC