[PATCH] D139037: [RISCV] Fold low 12 bits into instruction during frame index elimination

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:305
+      MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Lo12);
+      Offset = StackOffset::get(SignExtend64<32>(Hi20 << 12), Offset.getScalable());
     }
----------------
craig.topper wrote:
> reames wrote:
> > craig.topper wrote:
> > > Can this just be
> > > 
> > > `SignExtend64<32>(Val - Lo12)`?
> > See comment about being really bad at bitmath.  I tried to follow the constant materialization code as closely as possible.  :)
> > 
> > If you think this is correct, I'll make the change and report any differences I spot.  
> Thinking about it more, I'm not sure the SignExtend64<32> is correct.   SignExtend<32>(Hi20 << 12) + SignExtend<12>(Lo) is not guaranteed to give back the original constant.
> 
> Val - Lo12 without a SignExtend64<32> would guarantee we'd produce the original value, but Val - Lo12 isn't guaranteed to be a simm32. Does later code require the offset to be simm32?
It would need to be `(uint64_t)Val - (uint64_t)Lo12` to avoid UB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139037