[PATCH] D18890: [AArch64] add SSA Load Store optimization pass

[Jun Bum Lim via llvm-commits]

junbuml added a comment.

Hi Jongwon,

Sorry it's taken little long to revisit. Please see my inline comments.


================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:117
@@ +116,3 @@
+// the instruction located later between MIa and MIb.
+bool AArch64SSALoadStoreOpt::hasAliasBetween(MachineInstr *MIa,
+                                             MachineInstr *MIb) {
----------------
This function basically assume that MIa and MIb is in the same basic block. But, isn't it possible that  MIa and MIb could be in different basic blocks?  

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:123-136
@@ +122,16 @@
+  bool IsStartMIMet = false;
+  for (auto &MBBI : *MBB) {
+    MachineInstr *MI = &MBBI;
+
+    // When neither MIa nor MIb is not met, check if MI is either MIa or MIb.
+    // If met, set MI to EndMI.
+    if (!IsStartMIMet) {
+      if (MI == MIa) {
+        IsStartMIMet = true;
+      } else if (MI == MIb) {
+        EndMI = MIa;
+        IsStartMIMet = true;
+      }
+      continue;
+    }
+
----------------
I believe you could do something like this : 

for (auto &MBBI : make_range(MIa->getIterator(), MIb->getIterator()))

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:144
@@ +143,3 @@
+      return false;
+    } else if (MI->mayStore()) {
+      // Check if MI is aliased with EndMI.
----------------
When you check alias between two stores you should also check if there is any  load aliased with the second store if you always move the second to the first.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:406-424
@@ +405,21 @@
+      // instruction.
+      if (instrAliased(ConsecutiveStores[0], ConsecutiveLoads[1])) {
+        DEBUG(dbgs() << "The first store instruction:\n    ");
+        DEBUG(ConsecutiveStores[0]->print(dbgs()));
+        DEBUG(dbgs()
+              << "might be aliased with the second load instruction:\n    ");
+        DEBUG(ConsecutiveLoads[1]->print(dbgs()));
+        return false;
+      }
+
+      // The second store instruction should not be aliased to the first load
+      // instruction.
+      if (instrAliased(ConsecutiveStores[1], ConsecutiveLoads[0])) {
+        DEBUG(dbgs() << "The second store instruction:\n    ");
+        DEBUG(ConsecutiveStores[1]->print(dbgs()));
+        DEBUG(dbgs()
+              << "might be aliased with the first load instruction:\n    ");
+        DEBUG(ConsecutiveLoads[0]->print(dbgs()));
+        return false;
+      }
+
----------------
I don't think you need to have these alias checks specifically because you check alias between two loads and two stores below. 

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:497
@@ +496,3 @@
+  for (auto &MBBI : MBB) {
+    MachineInstr *MI = &MBBI;
+    switch (MI->getOpcode()) {
----------------
It seems that you don't handle volatile. You could use isCandidateToMergeOrPair().


http://reviews.llvm.org/D18890