[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);
----------------
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.)


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2282
-    // Ditto.
-    Align Alignment = CP->getAlign();
-    if (Offset2 != 0 && Alignment <= Offset2)
----------------
This seems like we should be able to write a test for this.

Maybe a large constant which is used twice - once in full, and then the second using only some upper bits?

If you don't mind, I'd prefer to have the deletion of untested code done in a separate change than the move of the global handling.  Just for clarity of revert attribution if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128738