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

Fri Jul 1 09:35:14 PDT 2022

craig.topper 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:
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128738