Wasmtime GitHub notifications bot (Jan 16 2023 at 22:00): Wasmtime GitHub notifications bot (Jan 16 2023 at 22:00):afonso360 edited PR #5578 from fix-riscv64-imm to main:
:wave: Hey,
This PR switches the riscv64 backend to use the constant pool for loading 32 and 64 bit constants. We only do this in the case where we can't simply use a
lui/addiinstruction to get the right constant.It also adds a Label based addressing mode for loads and stores.
Fixes #5569
My understanding of this issue is that a veneer would be emitted right in the middle of a LoadConst sequence and since we used hardcoded relative offsets, these would break and point to the wrong location. Switching to a label based system prevents this kinds of issues.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:09):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:09):bjorn3 created PR review comment:
Why use a constant pool for 32bit values? Loading from the constant pool requires auipc+lw while loading a constant requires lui+addiw which is exactly the same size in terms of instructions but doesn't require an extra 4 bytes for the constant pool entry.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:13):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:13):bjorn3 created PR review comment:
This label doesn't distinguish between two different constants.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:14):bjorn3 edited PR review comment.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:15):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:15):afonso360 created PR review comment:
We do try to use
lui+addiinInst::load_constant_u32and fall back here if those for some reason can't load it. Although I'm not entirely sure if that ever happens.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:17):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:17):bjorn3 created PR review comment:
That shouldn't be possible. 20 for lui + 12 for addiw == 32.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:18):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:18):afonso360 created PR review comment:
I don't really have a very good solution for this, we only know which label we get assigned after we emit the constant into the pool, and that only happens at emit time. When formatting the code we don't know which one its going to end up in.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:24):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:26):afonso360 created PR review comment:
That's right, and I think that just found a bug with our constant emissions, I tried to assert that we would always emit
lui+addiand that assert failed! I'm going to investigate further, but yeah removing the LoadConst32 seems like the way to go.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:26):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:27):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:37):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Jan 16 2023 at 22:37):bjorn3 created PR review comment:
Oh, the actual immediate is put between parens. I read the parens as being part of the regular lw inst syntax, but in that case it would be more like
lw x2, 0xdeadbeef(x1), so my brain still parsed it wrong.
Wasmtime GitHub notifications bot (Jan 17 2023 at 09:21):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Jan 17 2023 at 15:35):afonso360 updated PR #5578 from fix-riscv64-imm to main.
Wasmtime GitHub notifications bot (Jan 17 2023 at 15:37):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Jan 17 2023 at 15:37):afonso360 created PR review comment:
It turns out that riscv has
li rd, immassembler mnemonic that is much mor readable. I've switched to using that.
Wasmtime GitHub notifications bot (Jan 17 2023 at 15:38):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Feb 09 2023 at 14:37):afonso360 closed without merge PR #5578.
Last updated: Dec 23 2024 at 12:05 UTC