Stream: git-wasmtime

Topic: wasmtime / PR #7057 riscv64: Add compressed `addi`

Wasmtime GitHub notifications bot (Sep 18 2023 at 19:54):

afonso360 opened PR #7057 from afonso360:riscv-c-inst-part-2 to bytecodealliance:main:

:wave: Hey,

This PR introduces all of the compressed variations of the addi instruction (4 different adds!) as well as compressed trap, compressed debugtrap, and compressed shift left.

We have the following add variants:

• c.addi this is the normal addi, with a signed 6 bit immediate field.
• c.addiw also fairly normal, a 32 bit add with a 6 bit immediate, that sign extends the result to 64bits.
• c.addi16sp modifies the stack pointer with a signed 6 bit immediate scaled by 16.
• c.addi4spn adds a zero extended 6 bit immediate scaled by 4 to the stack pointer. Unlike c.addi16sp it allows saving the result in any compressible register.
  

Wasmtime GitHub notifications bot (Sep 18 2023 at 19:54):

afonso360 requested abrown for a review on PR #7057.

Wasmtime GitHub notifications bot (Sep 18 2023 at 19:54):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #7057.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:31):

alexcrichton created PR review comment:

To avoid the double-construction logic for Imm6::maybe_from_imm12 could this do:

let imm6 = match Imm6::maybe_from_imm12(imm12) {
    Some(imm6) => imm6,
    None => return false,
};

or alternatively if this function returned Option<()> instead of bool it could use ?. Or alternatively I think this could use let Some(imm6) = ... else { return false; };, but I haven't use let/else much myself.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:31):

alexcrichton created PR review comment:

Instead of gating all these rules with has_zca, could that be at the top of the function to early-return false if it's not enabled?

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:31):

alexcrichton created PR review comment:

Should this be c.addiw instead of c.sext.w?

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:44):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:44):

afonso360 created PR review comment:

Yes, I think we can. I was worried that it was possible for a target to implement Zcd or Zcb (which I'm planing on implementing soon) without Zca, but I looked at the spec and both of them require Zca to be present.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:44):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:44):

afonso360 created PR review comment:

Oops, yes it should.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:46):

alexcrichton submitted PR review:

Oh I meant to do this too

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:47):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:47):

afonso360 created PR review comment:

Here I think it's ok to do that, but I'd like to keep the separate check for c.addi16sp and c.addi4spn so that we can try to match all 3 instructions. Otherwise we could enter into one branch and never emit the instruction.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:48):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:53):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 18 2023 at 20:54):

afonso360 updated PR #7057.

Wasmtime GitHub notifications bot (Sep 18 2023 at 21:12):

afonso360 has enabled auto merge for PR #7057.

Wasmtime GitHub notifications bot (Sep 19 2023 at 00:16):

afonso360 merged PR #7057.

Last updated: Nov 22 2024 at 16:03 UTC