[llvm] [RISC-V] Fix crash with late stack realignment requirement (PR #83496)

Craig Topper via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 13 12:53:57 PDT 2024


topperc wrote:

> ...
> 
> > > I suppose we could solve it very pessimistically for the Global/CPI case by adding a lui/addi pair into the register that gets freed with the first load/store (and addi for the immediate case), but that seems worse than the potential stack re-alignment for the E ABI. Especially since we would presumably have to restore the address if the register isn't dead.
> > 
> > 
> > The Global/CPI case should be independent of the stack align. Those should only come from user loads/stores. I wonder if we should be splitting 64-bit loads/stores during isel instead of generating pseudos.
> 
> I'm not sure I fully follow how this will help resolve this problem. Do you mean that we would set the alignment of the paired registers to 4, expand the loads/stores of `f64` to the two loads/stores plus the necessary `build_pair`/split nodes? Then we can ensure that the loads/stores are able to access each of the pieces just as we do with any other load/store and if we end up spilling any pairs, we won't have the re-alignment issue?

Globals/CPI are unrelated to stack realignment issue let's forget about them.

When I mentioned `RISCVExpandPseudo::expandRV32ZdinxStore and RISCVExpandPseudo::expandRV32ZdinxLoad` I was referring to the else case that `assert(isInt<12>(MBBI->getOperand(2).getImm() + 4));` For pair load/stores created by spill/fill we have the alignment set to 2*XLen so that this assert can never fail even if the offset from SP is near the limit of simm12. The +4 will always be an OR for the stack spill since the spill address should be 8 byte aligned. This keeps it from generating a carry.

I wonder if we could change the alignment of the pair to 4, and scavenge a register in the rare case that `MBBI->getOperand(2).getImm()` is a simm12, but `MBBI->getOperand(2).getImm() + 4` isn't.

Or change the spill alignment to 4 and teach `eliminateFrameIndex` that it can't fold offset `>=2044` into the address of a pair load/store and needs to use a separate ADDI instead. It already knows to use an ADDI for `>2047` or `<-2048` for regular load/store.

https://github.com/llvm/llvm-project/pull/83496


More information about the llvm-commits mailing list