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

[Craig Topper via Phabricator via llvm-commits]

craig.topper created this revision.
craig.topper added reviewers: luismarques, asb, reames, jrtc27, efriedma.
Herald added subscribers: StephenFan, frasercrmck, ecnelises, apazos, sameer.abuasal, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, niosHD, sabuasal, simoncook, johnrusso, rbar, hiraditya.
Herald added a project: All.
craig.topper requested review of this revision.
Herald added subscribers: pcwang-thead, MaskRay.
Herald added a project: LLVM.

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 <https://reviews.llvm.org/D60294>.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124644

Files:
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
  llvm/test/CodeGen/RISCV/split-offsets.ll


Index: llvm/test/CodeGen/RISCV/split-offsets.ll
===================================================================
--- llvm/test/CodeGen/RISCV/split-offsets.ll
+++ llvm/test/CodeGen/RISCV/split-offsets.ll
@@ -118,3 +118,38 @@
   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
+}
Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -519,7 +519,9 @@
 
     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);
@@ -996,6 +998,7 @@
 
 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
@@ -1025,7 +1028,7 @@
     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
@@ -2447,7 +2450,7 @@
     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;
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D124644.425922.patch
Type: text/x-patch
Size: 3333 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220428/33f70ae2/attachment.bin>