[RISCV] Match RISCVISD::ADD_LO in SelectAddrRegImm.

[craig.topper]

Summary

This allows us to fold global and constant pool addresses into
load/store during isel instead of in the post-isel peephole. I
did not copy the alignment check for ConsantPoolSDNode because it
wasn't tested.

Remove the now untested GlobalAddressSDNode and ConsantPoolSDNode
handling from the peephole.

I think the only time the peephole is used now is when we split a
constant add into (addi (addi X, C1), C2) or (addi (add X, C3), C4). I plan to look at folding
that into SelectAddrRegImm next.

[craig.topper]

Rebase

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1924

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

2312

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.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1924

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.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1924

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.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2312

I'm not sure we share constant pools like that. When a constant pool is created in SelectionDAG, the offset is provided by the caller. It's not calculated based on there being some other constant pool entry with the same bit pattern in upper bits.

[reames]

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

1924

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.

[craig.topper]

Use commonAlignment to match the recent change to the post-isel peephole.
Don't delete any code from the post-isel peephole.

[reames]

LGTM, remember to update your submission comment per changes.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

 bool RISCVDAGToDAGISel::SelectAddrRegImm(SDValue Addr, SDValue &Base,
                                          SDValue &Offset) {
   if (SelectAddrFrameIndex(Addr, Base, Offset))
     return true;
 
   SDLoc DL(Addr);
   MVT VT = Addr.getSimpleValueType();
 
+  if (Addr.getOpcode() == RISCVISD::ADD_LO) {
+    Base = Addr.getOperand(0);
+    Offset = Addr.getOperand(1);
+    return true;
+  }
+
   if (CurDAG->isBaseWithConstantOffset(Addr)) {
     int64_t CVal = cast<ConstantSDNode>(Addr.getOperand(1))->getSExtValue();
     if (isInt<12>(CVal)) {
       Base = Addr.getOperand(0);
+      if (Base.getOpcode() == RISCVISD::ADD_LO) {
+        SDValue LoOperand = Base.getOperand(1);
+        if (auto *GA = dyn_cast<GlobalAddressSDNode>(LoOperand)) {
+          // If the Lo in (ADD_LO hi, lo) is a global variable's address
+          // (its low part, really), then we can rely on the alignment of that
+          // variable to provide a margin of safety before low part can overflow
+          // the 12 bits of the load/store offset. Check if CVal falls within
+          // that margin; if so (low part + CVal) can't overflow.
+          const DataLayout &DL = CurDAG->getDataLayout();
+          Align Alignment = commonAlignment(
+              GA->getGlobal()->getPointerAlignment(DL), GA->getOffset());
+          if (CVal == 0 || Alignment > CVal) {
+            int64_t CombinedOffset = CVal + GA->getOffset();
+            Base = Base.getOperand(0);
+            Offset = CurDAG->getTargetGlobalAddress(
+                GA->getGlobal(), SDLoc(LoOperand), LoOperand.getValueType(),
+                CombinedOffset, GA->getTargetFlags());
+            return true;
+          }
+        }
+      }
+
       if (auto *FIN = dyn_cast<FrameIndexSDNode>(Base))
         Base = CurDAG->getTargetFrameIndex(FIN->getIndex(), VT);
       Offset = CurDAG->getTargetConstant(CVal, DL, VT);
       return true;
     }
   }
 
   // Handle ADD with large immediates.
if (Offset2 != 0 && Alignment <= Offset2)
       return false;
     int64_t Offset1 = GA->getOffset();
     int64_t CombinedOffset = Offset1 + Offset2;
     ImmOperand = CurDAG->getTargetGlobalAddress(
         GA->getGlobal(), SDLoc(ImmOperand), ImmOperand.getValueType(),
         CombinedOffset, GA->getTargetFlags());
   } else if (auto *CP = dyn_cast<ConstantPoolSDNode>(ImmOperand)) {
     // Ditto.
     Align Alignment = commonAlignment(CP->getAlign(), CP->getOffset());
     if (Offset2 != 0 && Alignment <= Offset2)
       return false;
     int64_t Offset1 = CP->getOffset();
     int64_t CombinedOffset = Offset1 + Offset2;
     ImmOperand = CurDAG->getTargetConstantPool(
         CP->getConstVal(), ImmOperand.getValueType(), CP->getAlign(),
         CombinedOffset, CP->getTargetFlags());
   } else {