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

[Junmo Park via llvm-commits]

flyingforyou added a comment.

Thanks JunBum.

Your comment is very helpful for me. 
I think that next commit is also insufficient, too.
So please review the next commit also, if you have time.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:189
@@ +188,3 @@
+static bool isAdjacentLdStMerge(MachineInstr *MI, unsigned Limit) {
+  if (!MI->mayStore() &&
+      !MI->mayLoad() &&
----------------
junbuml wrote:
> You can use clang-format to check your coding style.
I will fix it.
I didn't know clang-format. It's very convinient.

Thanks Junbum.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:195-198
@@ +194,6 @@
+
+  if (Limit == 1)
+    return true;
+
+  return false;
+}
----------------
junbuml wrote:
> You can replace this with return Limit == 1; 

Yes. You're right.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:842
@@ +841,3 @@
+        // memory reference information which treated volatile access.
+        if ((!isAdjacentLdStMerge(MI, Limit) && MI->hasOrderedMemoryRef()) ||
+             TII->isLdStPairSuppressed(MI))
----------------
junbuml wrote:
> Do you need to find a paired until you encounter the first memory operation? Or did you intend to check just right next instruction?  
> 
> Looks like you try to see just right next instruction. Right? If that's the case you could use Count as well. 
> 
I just want to check right next instruction.
I think adjacent or just next to.. these expressions are not enough for explanation of my patch.

I will change comments and function name also.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1160-1161
@@ -1139,3 +1159,4 @@
+  bool isVolatile = false;
   if (MI->hasOrderedMemoryRef())
-    return false;
+    isVolatile = true;
 
----------------
junbuml wrote:
> I think you can move this check in findMatchingInsn().
Ok. I will.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1176-1178
@@ +1175,5 @@
+  if (isVolatile) {
+    // Only check the next instruction, if this is volatile load/store.
+    // We just pass 1 for ScanLimit value. That means we just check the
+    // next instruction for merging.
+    Paired = findMatchingInsn(MBBI, Flags, 1);
----------------
junbuml wrote:
> Can you add little bit more detail about why you want to check just the right next instruction ?
After moving "hasOrderedMemoryRef", I will add more comment inside of findMatchingInsn function.



http://reviews.llvm.org/D14489