riscv64: Load constants from the constant pool #6933
Conversation
Move that logic into `LoadInlineConst`
We were accidentally overriding the offset with the base.
afonso360
added
the
cranelift:area:riscv64
There was a problem hiding this comment.
Nice! One comment about something that's perhaps preexisting, but this looks good to me regardless of that.
Constants that are observed at emit time are still loaded inline since I couldn't find a way of registering them with the MachBuffer for loading.
My guess is that if possible it'd be best to break up the macro-instruction into its components to handle this. That way instead of using macro-instructions the helpers in ISLE can be used. This may not always be applicable though.
I did look at a few of these in emit.rs though and they were related to things like sp adjustments and stack probe loops which I think could also reasonably be switched over to asserting that the constant fits in an instruction rather than requiring a pool. (so long as the range matches what one would reasonably expect for these situations)
| (rule 0 (imm (ty_int ty) c) | ||
| (emit_load | ||
| (LoadOP.Ld) | ||
| (mem_flags_trusted) | ||
| (gen_const_amode (emit_u64_le_const c)))) |
There was a problem hiding this comment.
This is preexisting I think, and probably affects other backends too, but technically emitting a 64-bit constant here is over-approximating how large it needs to be, right? In that only the ty-width sized bit-pattern of c is all that matters so for a 32-bit constant the 64-bit size is overkill?
No need to fix here since this preserves what was done before, but may be worth trying to fix later?
There was a problem hiding this comment.
My plan is to eventually fix #6922 and then we should be able to change this to only ever trigger for 64 bit constants, since all other constants should be able to be emitted with the previous rules (i.e. with constant materialization instructions).
We already sort of do this. We have I'd eventually like to try and fully merge the load constant logic with some ISLE rules, but I'm not entirely sure how to do that. Right now both |
|
Makes sense! I was thinking that the "solution" there is to slowly whittle away at callers to |
* riscv64: Use `Inst::load_u64_constant` when loading load offsets * riscv64: Load ISLE constants from pool * riscv64: Delete `offset32_imm` * riscv64: Delete `pack_float_rounding_mode` * riscv64: Delete `LoadConst32` instruction * riscv64: Delete LoadConstant code Move that logic into `LoadInlineConst` * riscv64: Fix `LoadAddr` instructions for RegOffset We were accidentally overriding the offset with the base. * riscv64: Update inst_worst_case_size
👋 Hey,
This is a follow up to #6915 (and #5578 I guess). This PR enables loading ISLE constants from the constant pool.
Not all constants are loaded from the constant pool. Constants that are observed at emit time are still loaded inline since I couldn't find a way of registering them with the MachBuffer for loading.
This also causes a slight pessimization of the dynamic instruction count since we currently force the address to be fully loaded into a register before a load when resolving labels. This is easily fixable, but I'm going to do it in a follow-up PR.