[PATCH] D23814: AMDGPU/SI: Improve SILoadStoreOptimizer and run it before the scheduler

[Tom Stellard via llvm-commits]

tstellarAMD added inline comments.

================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:107
@@ -102,6 +106,3 @@
     AU.setPreservesCFG();
-    AU.addPreserved<SlotIndexes>();
-    AU.addPreserved<LiveIntervals>();
-    AU.addPreserved<LiveVariables>();
-    AU.addRequired<LiveIntervals>();
+    AU.addRequired<AAResultsWrapperPass>();
 
----------------
arsenm wrote:
> Does this need to be required? If it's not required is it available in the current pass pipeline? I think there's a subtarget hook we needed to enable to use this
This is required so we don't regress tests in llvm.memcpy.ll, The alias analysis passes are always run in TargetPassConfig::addIRPasses().  The subtarget hook looks like it is only used by the SelectionDAG passes.

================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:226-249
@@ -189,1 +225,26 @@
+
+      // When we match I with another DS instruction we will be moving I down
+      // to the location of the matched instruction any uses of I will need to
+      // be moved down as well.
+      for (const MachineOperand *Def : DefsToMove) {
+        std::pair<bool, bool> ReadWrite =
+            MBBI->readsWritesVirtualRegister(Def->getReg());
+        if (ReadWrite.second)
+          // This means Def and MBBI both write to the same super register.
+          // FIXME: This is probably OK, but we may need some additional logic
+          // to make sure the sub-indices of the writes don't overlap.
+          return E;
+
+        // If ReadWrite.first is true, then there is a use of Def between I
+        // and the instruction that I will potentially be merged with, we
+        // will need to move this instruction after the merged instructions.
+        if (ReadWrite.first) {
+          InstsToMove.push_back(&*MBBI);
+          addDefsToList(*MBBI, DefsToMove);
+          break;
+        }
+      }
+      continue;
+    }
 
+    // Don't merge volatiles.
----------------
arsenm wrote:
> It is not possible to have a sub register def in SSA so I don't think you need any of this
I dropped the code that checks for writes, but the code that checks for reads doesn't have anything to do with sub-registers.


https://reviews.llvm.org/D23814