alexcrichton requested afonso360 for a review on PR #7121.
alexcrichton opened PR #7121 from alexcrichton:rv64-extends
to bytecodealliance:main
:
This PR is a long series of commits which is a culmination of some investigation and poking around I've been doing after taking a look at https://github.com/bytecodealliance/wasmtime/pull/7113. Previously there were quite a few entry points into the core
extend
function in the riscv64 backend which all had a number of different concerns handled and felt, at least from my perspective, a bit confusing to follow. The goal of this refactoring was to canonicalize around two helper functions,zext
andsext
, which take no arguments except aValue
. The functions would then internally do whatever is necessary to extend to the full register width and anXReg
would be returned.This required quite a bit of refactoring and changes to the previous code such as:
- Handling of 128-bit values was pushed into callers that needed it to avoid needing to have all callers deal with the possibility of 128-bit values.
- Many functions were moved around to be able to pass in a
Value
vs aXReg Type
combo. This involved a lot of movement frominst.isle
intolower.isle
as well as code motion.- Many helpers which called each other now bottom out much more quickly or have a little bit more duplication in
lower.isle
. Personally I felt the duplication was relatively small and ok, but other reviews are welcome!- All extension-related helpers are removed now except
zext
andsext
which internally have all conditional logic as necessary.The final culmination of this PR is that the assembly output probably doesn't change all that much for the time being. There's now a clear place, however, to skip sign extension based on the structure of a
Value
. For example I'd like to add support for theaddw
instruction which would skip the need to sign-extend the result of a 32-bit add. Similarly for other 32-bit operations as necessary. This is all in preparation for optimizingicmp
which always has to sign extend so the goal in theory is to remove as many sign extension as possible.
alexcrichton requested fitzgen for a review on PR #7121.
alexcrichton requested wasmtime-compiler-reviewers for a review on PR #7121.
afonso360 submitted PR review:
I really like this! Thanks for these cleanups!
I like the
val_already_extended
approach, and I think we may be able to fit a few more ops in it likefcmp
,{s,u}load{8,16,32}
andbmask
(with some restrictions)! I might do a follow up PR if you don't do it first.For example I'd like to add support for the addw instruction which would skip the need to sign-extend the result of a 32-bit add. Similarly for other 32-bit operations as necessary.
I should note that we already have support for these operations, but they are all in the
sextend
part oflower.isle
.Although I think we might be missing some, like the
rolw
orrorw
that sign extend the results.And looking that the
uextend
branch we are also missing a bunch of them likecpopw
/clzw
/ctzw
.I don't know if it's better to have them next to
{s,u}extend
or their original ops.
afonso360 submitted PR review:
I really like this! Thanks for these cleanups!
I like the
val_already_extended
approach, and I think we may be able to fit a few more ops in it likefcmp
,{s,u}load{8,16,32}
andbmask
(with some restrictions)! I might do a follow up PR if you don't do it first.For example I'd like to add support for the addw instruction which would skip the need to sign-extend the result of a 32-bit add. Similarly for other 32-bit operations as necessary.
I should note that we already have support for these operations, but they are all in the
sextend
part oflower.isle
.Although I think we might be missing some, like the
rolw
orrorw
that sign extend the results.And looking that the
uextend
branch we are also missing a bunch of them likecpopw
/clzw
/ctzw
.I don't know if it's better to have them next to
{s,u}extend
or their original ops since it might be easy to miss them where they are right now.
afonso360 submitted PR review:
I really like this! Thanks for these cleanups!
I like the
val_already_extended
approach, and I think we may be able to fit a few more ops in it likefcmp
,{s,u}load{8,16,32}
andbmask
(with some restrictions)! I might do a follow up PR if you don't do it first.For example I'd like to add support for the addw instruction which would skip the need to sign-extend the result of a 32-bit add. Similarly for other 32-bit operations as necessary.
I should note that we already have support for these operations, but they are all in the
sextend
part oflower.isle
.Although I think we might be missing some, like the
rolw
orrorw
that sign extend the results.And looking at the
uextend
rules we are also missing a bunch of them likecpopw
/clzw
/ctzw
.I don't know if it's better to have them next to
{s,u}extend
or their original ops since it might be easy to miss them where they are right now.
afonso360 submitted PR review:
I really like this! Thanks for these cleanups!
I like the
val_already_extended
approach, and I think we may be able to fit a few more ops in it likefcmp
,{s,u}load{8,16,32}
andbmask
(with some restrictions)! I might do a follow up PR if you don't do it first.For example I'd like to add support for the addw instruction which would skip the need to sign-extend the result of a 32-bit add. Similarly for other 32-bit operations as necessary.
I should note that we already have support for these operations, but they are all in the
sextend
part oflower.isle
.Although I think we might be missing some, like the
rolw
orrorw
that sign extend the results.And looking at the
uextend
rules we are also missing a bunch of them likecpopw
/clzw
/ctzw
.I don't know if it's better to have them next to
{s,u}extend
or their original ops since it might be easy to miss them where they are right now.
Edit: Maybe we could add a
maybe_sextend
/maybe_uextend
matcher and then these rules could be folded into the regular$I32
rules.
alexcrichton merged PR #7121.
Last updated: Dec 23 2024 at 12:05 UTC