[PATCH] D14489: [AArch64] Applying load pair optimization for volatile load/store

[Jun Bum Lim via llvm-commits]

junbuml added inline comments.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:189
@@ +188,3 @@
+static bool isRightNextLdStMerge(MachineInstr *MI, unsigned Limit) {
+  if (!MI->mayStore() && !MI->mayLoad() && !MI->isCall() &&
+      !MI->hasUnmodeledSideEffects())
----------------
Do you need to check this again? If this function is called only in findMatchingInsn,  assert will be better.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:195
@@ +194,3 @@
+
+  return false;
+}
----------------
Please remove this.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:849-853
@@ -822,2 +848,7 @@
         // safely transform. Similarly, stop if we see a hint to avoid pairs.
-        if (MI->hasOrderedMemoryRef() || TII->isLdStPairSuppressed(MI))
+        // But if the Limit is 1 which means we try to merge just right next
+        // load/store instruction, we can merge volatile load/store.
+        // There is no semantic difference even if the order is changed.
+        // Pairwise instruction doesn't have memory reference information
+        // which treated volatile access.
+        if ((!isRightNextLdStMerge(MI, Limit) && MI->hasOrderedMemoryRef()) ||
----------------
Why merging with the next instruction is safe. Is this also safe for the narrow load merge    case where we concatenate MOMs  ? 

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:854
@@ +853,3 @@
+        // which treated volatile access.
+        if ((!isRightNextLdStMerge(MI, Limit) && MI->hasOrderedMemoryRef()) ||
+            TII->isLdStPairSuppressed(MI))
----------------
I doubt if isRightNextLdStMerge is the right function name? Something specifically volatile will be better ? 

================
Comment at: test/CodeGen/AArch64/arm64-ldp.ll:364
@@ +363,3 @@
+; CHECK: ldr
+define i64 @volatile_ldp_long(i64* %p) nounwind {
+  %add.ptr = getelementptr inbounds i64, i64* %p, i64 0
----------------
It will be good to add more tests to cover all other types of loads/stores impacted by this patch, not just i64.


http://reviews.llvm.org/D14489