Stream: git-wasmtime

Topic: wasmtime / PR #7208 riscv64: Fix encoding for `c.addi4spn`

Wasmtime GitHub notifications bot (Oct 10 2023 at 14:57):

afonso360 opened PR #7208 from afonso360:riscv-fix-addi4spn-encoding to bytecodealliance:main:

:wave: Hey,

I was running fuzzgen on riscv when it crashed with an illegal instruction. When generating large stack offsets, we can use the compressed instruction c.addi4spn, It has a range of {4,1020} bytes. However we were accidentally not encoding the MSB on the immediate field.

This meant that the instruction would be encoded with an offset of 0, which is illegal for this instruction. The fix here is to ensure that the MSB bit is encoded correctly.

Wasmtime GitHub notifications bot (Oct 10 2023 at 14:57):

afonso360 requested fitzgen for a review on PR #7208.

Wasmtime GitHub notifications bot (Oct 10 2023 at 14:57):

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

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:05):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:05):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:05):

fitzgen created PR review comment:

Do we have emit_tests.rs for riscv64? I would kind of expect some updated tests in there that exercise this, rather than just the checked in fuzz test.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:37):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:37):

afonso360 created PR review comment:

I've been using the disassembled section in the compile tests as a check that the encodings are correct. However, we don't have many compile tests specifically for C instructions. Most of them test only with the Base ISA and don't enable the other extensions so they won't be emitting this instruction.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:38):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 10 2023 at 17:48):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 10 2023 at 19:30):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2023 at 19:30):

fitzgen created PR review comment:

Right, I guess before merging this I would like to have either a test in emit_tests.rs that does some loop over min/max/etc values for this type of immediate and encodes each of them or something else like that in spirit (e.g. a series of c.addi4spn instructions with various min/max/etc values in a filetest).

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

afonso360 updated PR #7208.

Wasmtime GitHub notifications bot (Oct 11 2023 at 18:16):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2023 at 18:16):

afonso360 created PR review comment:

I've added a test for both min and max values as well as one that sets a bit in each subfield in the immediate of the instruction. I've also added tests for out of bound values.

I added them into zca.clif instead of emit_tests.rs, since its slightly awkward to add these there because they are encoding only changes we don't actually know the name of the instruction in cranelift. So the test would look something like addi a1, sp, 4 but the result is a 2 byte c.addisp4n.

Wasmtime GitHub notifications bot (Oct 11 2023 at 19:51):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Oct 11 2023 at 20:36):

fitzgen merged PR #7208.

Last updated: Oct 23 2024 at 20:03 UTC