Stream: git-wasmtime

Topic: wasmtime / PR #7121 riscv64: Refactor and enable optimizi...

Wasmtime GitHub notifications bot (Oct 01 2023 at 19:36):

alexcrichton requested afonso360 for a review on PR #7121.

Wasmtime GitHub notifications bot (Oct 01 2023 at 19:36):

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 and sext, which take no arguments except a Value. The functions would then internally do whatever is necessary to extend to the full register width and an XReg 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 a XReg Type combo. This involved a lot of movement from inst.isle into lower.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 and sext 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 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. This is all in preparation for optimizing icmp which always has to sign extend so the goal in theory is to remove as many sign extension as possible.

Wasmtime GitHub notifications bot (Oct 01 2023 at 19:36):

alexcrichton requested fitzgen for a review on PR #7121.

Wasmtime GitHub notifications bot (Oct 01 2023 at 19:36):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #7121.

Wasmtime GitHub notifications bot (Oct 02 2023 at 11:10):

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 like fcmp, {s,u}load{8,16,32} and bmask (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 of lower.isle.

Although I think we might be missing some, like the rolw or rorw that sign extend the results.

And looking that the uextend branch we are also missing a bunch of them like cpopw / clzw / ctzw.

I don't know if it's better to have them next to {s,u}extend or their original ops.

Wasmtime GitHub notifications bot (Oct 02 2023 at 11:12):

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 like fcmp, {s,u}load{8,16,32} and bmask (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 of lower.isle.

Although I think we might be missing some, like the rolw or rorw that sign extend the results.

And looking that the uextend branch we are also missing a bunch of them like cpopw / 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.

Wasmtime GitHub notifications bot (Oct 02 2023 at 11:13):

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 like fcmp, {s,u}load{8,16,32} and bmask (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 of lower.isle.

Although I think we might be missing some, like the rolw or rorw that sign extend the results.

And looking at the uextend rules we are also missing a bunch of them like cpopw / 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.

Wasmtime GitHub notifications bot (Oct 02 2023 at 11:15):

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 like fcmp, {s,u}load{8,16,32} and bmask (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 of lower.isle.

Although I think we might be missing some, like the rolw or rorw that sign extend the results.

And looking at the uextend rules we are also missing a bunch of them like cpopw / 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.

Wasmtime GitHub notifications bot (Oct 02 2023 at 17:13):

alexcrichton merged PR #7121.

Last updated: Nov 22 2024 at 16:03 UTC