riscv64: Use Constant Pool to load 32 and 64 bit constants#5578
riscv64: Use Constant Pool to load 32 and 64 bit constants#5578afonso360 wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
github-actions
bot
added
the
cranelift
afonso360
changed the title
| let data = imm.to_le_bytes(); | ||
|
|
||
| // Emit the constant into the pool | ||
| sink.defer_constant(label_data, 4, &data[..], u32::MAX); |
There was a problem hiding this 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.
There was a problem hiding this comment.
We do try to use lui+addi in Inst::load_constant_u32 and fall back here if those for some reason can't load it. Although I'm not entirely sure if that ever happens.
There was a problem hiding this comment.
That shouldn't be possible. 20 for lui + 12 for addiw == 32.
There was a problem hiding this 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+addiw and that assert failed! I'm going to investigate further, but yeah removing the LoadConst32 instruction seems like the way to go.
|
|
||
| ; block0: | ||
| ; auipc a0,0; ld a0,12(a0); j 12; .8byte 0xffff00000000 | ||
| ; auipc a0,.LConst; ld a0,.LConst(0xffff00000000) |
There was a problem hiding this comment.
This label doesn't distinguish between two different constants, right? Or do I read it wrong?
There was a problem hiding this 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.
Edit: Also, this label is not the actual label that ends up being used, this is sort of our internal assembly which for some ops is not real riscv assembly.
There was a problem hiding this 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.
There was a problem hiding this comment.
It turns out that riscv has li rd, imm assembler mnemonic that is much more readable. I've switched to using that.
afonso360
force-pushed
the
fix-riscv64-imm
branch
from
853f1e2 to
98bb9f7
Compare
|
@afonso360 it looks like the linked issue (#5569) has been resolved another way; do you still think you want to pursue this PR? |
|
I still think it would be a worthwhile improvement to the RISCV backend, however I don't really have time to work on this right now. I'll send a new PR once It's ready to merge. |
3 participants
👋 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.