[PATCH] D128738: [RISCV] Match RISCVISD::ADD_LO in SelectAddrRegImm.

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1895
+          // that margin; if so (low part + CVal) can't overflow.
+          const DataLayout &DL = CurDAG->getDataLayout();
+          Align Alignment = GA->getGlobal()->getPointerAlignment(DL);
----------------
craig.topper wrote:
> craig.topper wrote:
> > reames wrote:
> > > Er, correct me if I'm wrong here, but don't we need to be checking the combined offset for safety against the alignment?  If the original GV had a base aligned to 16, and an offset of 15, wouldn't adding a new offset of 15 cause problems?  We could in theory adjust the base, but do we actually do that?
> > > 
> > > (If this concern is valid, the original code had it too.)
> > I think you're right. As far as I know GA->getOffset() is always 0 which is probably why it hasn't caused a problem.
> Well partially right. I think we need to calculate commonAlignment(GA->getGlobal()->getPointerAlignment(DL), GA->getOffset()) and make sure CVal is less than that.
> 
> What we end up with is an LUI with the global adjusted by getOffset() and a load/store with the global adjusted by getOffset()+CVal. We need to ensure that the address calculated by the LUI is compatible with the address calculated by the load/store.
Thinking about this, I'm pretty sure that commonAlignment as you describe is sufficient, but not necessary.

If the combined offset is both less than 12 bits, and less than the alignment, then the globals low bits + the offset will be less than 12 bits.  (i.e. non-overlapping bits within the low bits)  Those bits could be folded either a) back into the ADD_LO or into the using load/store instruction.  Either should be legal, and both kill this use of the outer ADDI.    




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128738