Stream: git-wasmtime

Topic: wasmtime / PR #9114 Winch aarch64 extend, wrap, popcnt, p...

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:28):

vulc41n requested abrown for a review on PR #9114.

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:28):

vulc41n requested wasmtime-compiler-reviewers for a review on PR #9114.

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:28):

vulc41n opened PR #9114 from vulc41n:winch-aarch64-extend-wrap to bytecodealliance:main:

Hey :wave:

This PR implements extend, wrap, popcnt, promote & demote instructions for winch targeting aarch64.

#8321

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:28):

vulc41n requested alexcrichton for a review on PR #9114.

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:28):

vulc41n requested wasmtime-core-reviewers for a review on PR #9114.

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:44):

vulc41n updated PR #9114.

Wasmtime GitHub notifications bot (Aug 12 2024 at 16:47):

vulc41n updated PR #9114.

Wasmtime GitHub notifications bot (Aug 13 2024 at 11:52):

saulecabrera commented on PR #9114:

I can help reviewing this one.

Wasmtime GitHub notifications bot (Aug 13 2024 at 11:52):

saulecabrera requested saulecabrera for a review on PR #9114.

Wasmtime GitHub notifications bot (Aug 13 2024 at 14:51):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Aug 13 2024 at 14:51):

saulecabrera created PR review comment:

To reduce register pressure, could we use the scratch float register here?

Wasmtime GitHub notifications bot (Aug 13 2024 at 14:51):

saulecabrera created PR review comment:

In the case of aarch64 I believe that the fallback test is redundant i.e., since we are not generating an alternative sequence of instructions like in x64.

Wasmtime GitHub notifications bot (Aug 13 2024 at 14:51):

saulecabrera created PR review comment:

If I'm not wrong, Wasm only defined {i32, i64}_popcnt so I believe we only need to handle the S32 | S64 case and handle all the other operand sizes with unreachable!() instead of unimplemented, which also means that we can probably drop the addp_rrr implementation.

Wasmtime GitHub notifications bot (Aug 13 2024 at 14:51):

saulecabrera created PR review comment:

Could we add some methods to the ExtendKind implementation instead? Something like:

impl ExtendKind {
   fn signed(&self) -> bool { ... }
   fn from_bits(&self) -> u8 { ... }
   fn to_bits(&self) -> u8 { ... }
}

Wasmtime GitHub notifications bot (Aug 13 2024 at 22:59):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Aug 14 2024 at 09:30):

vulc41n submitted PR review.

Wasmtime GitHub notifications bot (Aug 14 2024 at 09:30):

vulc41n created PR review comment:

You're right, I used lower.isle as a reference but CLIF is different from WASM.

handle all the other operand sizes with unreachable!() instead of unimplemented

I just removed the match, please tell me if you think an assertion is needed

Wasmtime GitHub notifications bot (Aug 14 2024 at 09:30):

vulc41n updated PR #9114.

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:05):

saulecabrera submitted PR review:

One minor nit and this should be good to go.

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:05):

saulecabrera submitted PR review:

One minor nit and this should be good to go.

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:05):

saulecabrera created PR review comment:

The scratch register is manually tracked i.e., free_reg is a no-op in this case, so we can remove this call.

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:09):

vulc41n updated PR #9114.

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:11):

vulc41n updated PR #9114.

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:22):

saulecabrera submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Aug 14 2024 at 10:37):

saulecabrera merged PR #9114.

Last updated: Dec 23 2024 at 12:05 UTC