[DAGCombiner] reassociationCanBreakAddressingModePattern should check uses of the outer add.

[craig.topper]

Summary

When looking for memory uses,
reassociationCanBreakAddressingModePattern should check uses of
the outer ADD rather than the inner ADD. We want to know if the
two ops we're reassociating are used by a load/store.

In practice, the existing check usually works because CodeGenPrepare
will make one of the load/stores have an offset of 0 relative to
split GEP. That will make the inner add have a memory use.

To test this, I've manually split the GEPs so there is no 0 offset
store.

This issue was recently discussed in the original review D60294.

[luismarques]

LGTM. Thanks for the fix!

[luismarques]

@craig.topper Have you checked if CombinerHelper::reassociationCanBreakAddressingModePattern (in llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp) is sound?

[craig.topper]

In D124644#3487024, @luismarques wrote:

@craig.topper Have you checked if CombinerHelper::reassociationCanBreakAddressingModePattern (in llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp) is sound?

After a quick glance, I think it has the same issue. Was it copied from your original patch?

[luismarques]

In D124644#3487049, @craig.topper wrote:

After a quick glance, I think it has the same issue. Was it copied from your original patch?

I imagine it was inspired by it, to implement the GlobalISel equivalent? I didn't study it closely. Check https://reviews.llvm.org/D105069

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

private:
     SDValue visitVPOp(SDNode *N);
 
     SDValue visitFADDForFMACombine(SDNode *N);
     SDValue visitFSUBForFMACombine(SDNode *N);
     SDValue visitFMULForFMADistributiveCombine(SDNode *N);
 
     SDValue XformToShuffleWithZero(SDNode *N);
     bool reassociationCanBreakAddressingModePattern(unsigned Opc,
-                                                    const SDLoc &DL, SDValue N0,
+                                                    const SDLoc &DL,
+                                                    SDNode *N,
+                                                    SDValue N0,
                                                     SDValue N1);
     SDValue reassociateOpsCommutative(unsigned Opc, const SDLoc &DL, SDValue N0,
                                       SDValue N1);
     SDValue reassociateOps(unsigned Opc, const SDLoc &DL, SDValue N0,
                            SDValue N1, SDNodeFlags Flags);
 
     SDValue visitShiftByConstant(SDNode *N);
 

 static bool canSplitIdx(LoadSDNode *LD) {
   return MaySplitLoadIndex &&
          (LD->getOperand(2).getOpcode() != ISD::TargetConstant ||
           !cast<ConstantSDNode>(LD->getOperand(2))->isOpaque());
 }
 
 bool DAGCombiner::reassociationCanBreakAddressingModePattern(unsigned Opc,
                                                              const SDLoc &DL,
+                                                             SDNode *N,
                                                              SDValue N0,
                                                              SDValue N1) {
   // Currently this only tries to ensure we don't undo the GEP splits done by
   // CodeGenPrepare when shouldConsiderGEPOffsetSplit is true. To ensure this,
   // we check if the following transformation would be problematic:
   // (load/store (add, (add, x, offset1), offset2)) ->
   // (load/store (add, x, offset1+offset2)).
 
bool DAGCombiner::reassociationCanBreakAddressingModePattern(unsigned Opc,
   if (C1APIntVal.getBitWidth() > 64 || C2APIntVal.getBitWidth() > 64)
     return false;
 
   const APInt CombinedValueIntVal = C1APIntVal + C2APIntVal;
   if (CombinedValueIntVal.getBitWidth() > 64)
     return false;
   const int64_t CombinedValue = CombinedValueIntVal.getSExtValue();
 
-  for (SDNode *Node : N0->uses()) {
+  for (SDNode *Node : N->uses()) {
     auto LoadStore = dyn_cast<MemSDNode>(Node);
     if (LoadStore) {
       // Is x[offset2] already not a legal addressing mode? If so then
       // reassociating the constants breaks nothing (we test offset2 because
       // that's the one we hope to fold into the load or store).
       TargetLoweringBase::AddrMode AM;
       AM.HasBaseReg = true;
       AM.BaseOffs = C2APIntVal.getSExtValue();
if (isADDLike(N0, DAG) &&
         return DAG.getNode(ISD::ADD, DL, VT, N0.getOperand(0), Add0);
     }
   }
 
   if (SDValue NewSel = foldBinOpIntoSelect(N))
     return NewSel;
 
   // reassociate add
-  if (!reassociationCanBreakAddressingModePattern(ISD::ADD, DL, N0, N1)) {
+  if (!reassociationCanBreakAddressingModePattern(ISD::ADD, DL, N, N0, N1)) {
     if (SDValue RADD = reassociateOps(ISD::ADD, DL, N0, N1, N->getFlags()))
       return RADD;
 
     // Reassociate (add (or x, c), y) -> (add add(x, y), c)) if (or x, c) is
     // equivalent to (add x, c).
     // Reassociate (add (xor x, c), y) -> (add add(x, y), c)) if (xor x, c) is
     // equivalent to (add x, c).
     auto ReassociateAddOr = [&](SDValue N0, SDValue N1) {

llvm/test/CodeGen/RISCV/split-offsets.ll

while_body:
   store i32 %phi, i32* %gep1
   store i32 %i, i32* %gep2
   store i32 %phi, i32* %gep3
   br label %while_cond
 while_end:
   ret void
 }
 
+; GEPs have been manually split so the base GEP does not get used by any memory
+; instructions. Make sure we use a small offset in each of the stores.
+define void @test3([65536 x i32]* %t) {
+; RV32I-LABEL: test3:
+; RV32I:       # %bb.0: # %entry
+; RV32I-NEXT:    lui a1, 20
+; RV32I-NEXT:    addi a1, a1, -1920
+; RV32I-NEXT:    add a0, a0, a1
+; RV32I-NEXT:    li a1, 2
+; RV32I-NEXT:    sw a1, 4(a0)
+; RV32I-NEXT:    li a1, 3
+; RV32I-NEXT:    sw a1, 8(a0)
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: test3:
+; RV64I:       # %bb.0: # %entry
+; RV64I-NEXT:    lui a1, 20
+; RV64I-NEXT:    addiw a1, a1, -1920
+; RV64I-NEXT:    add a0, a0, a1
+; RV64I-NEXT:    li a1, 2
+; RV64I-NEXT:    sw a1, 4(a0)
+; RV64I-NEXT:    li a1, 3
+; RV64I-NEXT:    sw a1, 8(a0)
+; RV64I-NEXT:    ret
+entry:
+  %0 = bitcast [65536 x i32]* %t to i8*
+  %splitgep = getelementptr i8, i8* %0, i64 80000
+  %1 = getelementptr i8, i8* %splitgep, i64 4
+  %2 = bitcast i8* %1 to i32*
+  %3 = getelementptr i8, i8* %splitgep, i64 8
+  %4 = bitcast i8* %3 to i32*
+  store i32 2, i32* %2, align 4
+  store i32 3, i32* %4, align 4
+  ret void
+}