[PATCH] D126392: [RISCV] Use two ADDIs to do some stack pointer adjustments.

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM.

Minor optional comment from me, and remember to address nit from previous reviewer before landing.



================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:309
 
-    Register ScratchReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
-    TII->movImm(MBB, MBBI, DL, ScratchReg, Val, Flag);
-    BuildMI(MBB, MBBI, DL, TII->get(Opc), DestReg)
+  // Try to split the offset across two ADDIs. We need make sure we keep the
+  // stack pointer aligned after each ADDI. We need to determine the maximum
----------------
craig.topper wrote:
> reames wrote:
> > Is there anything here that guarantees stack alignment must be smaller than 12 bits?  If not, I don't think your statement about -2048 always being aligned holds.  
> > 
> > On the positive vs negative direction, shouldn't we always be able to use the same constant by simply swapping the ADD for a SUB?  
> As far as I know the stack align is part of the RISCVFrameLowering constructor. It doesn’t change per function. Unless someone changes it from 16 to a huge value it should be true.
> 
> There is no SUBI instruction.
> 
> 
I'd vague remembered there being more to the stack alignment on x86, and went and glanced.  That's specific to the "stackrealign" attribute which we don't appear to support on RISC-V.  I'm fine leaving that is for now.  

Do me a favor and add the assert that stack align is a reasonable value w.r.t. your chosen constant for future proofing this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126392/new/

https://reviews.llvm.org/D126392