Stream: git-wasmtime

Topic: wasmtime / PR #7045 riscv64: Refactor how immediates are ...

Wasmtime GitHub notifications bot (Sep 14 2023 at 23:30):

alexcrichton requested elliottt for a review on PR #7045.

Wasmtime GitHub notifications bot (Sep 14 2023 at 23:30):

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

Wasmtime GitHub notifications bot (Sep 14 2023 at 23:30):

alexcrichton opened PR #7045 from alexcrichton:refactor-riscv64-immediates to bytecodealliance:main:

I was curious to poke around the riscv64 backend and I wanted to touch up the handling of Imm{12,20} a bit after reading it. This commit is a refactoring of these two types with the following changes:

• The payload of these types is now unsigned and guarantees that irrelevant bits are set to zero. For example Imm12 is stored as u16 where the upper four bits are guaranteed to be zero. This fixes a discrepancy where Imm12::maybe_from_i64 was masked for example but Imm12::from_bits wasn't.

• The Neg for Imm12 impl was removed because -2048 is a valid Imm12 but 2048 is not in-range for Imm12 meaning that it is not an infallible operation.

• Accessors are now named bits to get the u32 representation suitable to be encoded into an instruction. Acquiring the underlying value is now done with as_i{16,32} depending on the type. The signed accessor does sign-extension as required to produce the semantically equivalent value.

• Manual constructors were renamed to from_{i16,i32} instead of from_bits. This in theory helps convey that they're constructors for logical values rather than literal bit-wise values. Additionally asserts are now placed in these constructors asserting that the provided value is in-range.

• The FALSE and TRUE constants were renamed ZERO and ONE and Imm20::ZERO was added.

This commit ended up changing many runtests, but only their CLIF printing rather than their encoding. This change is due to the fact that Display now prints the logical value of the immediate rather than the raw bit representation as a base 10 integer. It's not intended that this commit actually changes any behavior, instead it should purely be internal refactorings.

<!--
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 (Sep 14 2023 at 23:54):

alexcrichton updated PR #7045.

Wasmtime GitHub notifications bot (Sep 15 2023 at 09:12):

afonso360 submitted PR review:

LGTM! :+1: Thank you for looking into this. The Imm12 printing as unsigned has been slightly bothering me for a while now!

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:22):

alexcrichton has enabled auto merge for PR #7045.

Wasmtime GitHub notifications bot (Sep 15 2023 at 15:22):

alexcrichton updated PR #7045.

Wasmtime GitHub notifications bot (Sep 15 2023 at 16:43):

alexcrichton merged PR #7045.

Last updated: Oct 23 2024 at 20:03 UTC