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

[Junmo Park via llvm-commits]

flyingforyou added a comment.

Hi, Chad, Tim.

I concern too much about merging store instruction. I was just worried about that some hardware communicated through memory interface. Now, I change the code that can merge adjacent volatile store also.

If you think something is weired or insufficent, please tell me.

Thanks for reviewing.


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:818
@@ -817,3 +817,3 @@
         // safely transform. Similarly, stop if we see a hint to avoid pairs.
-        if (MI->hasOrderedMemoryRef() || TII->isLdStPairSuppressed(MI))
+        if ((Limit != 1 && MI->hasOrderedMemoryRef()) || TII->isLdStPairSuppressed(MI))
           return E;
----------------
mcrosier wrote:
> Please create a static helper function with comments suggesting why it's safe to merge adjacent loads/stores that are marked volatile.  Also, I don't think this check is sufficient.  The order of the offsets determine the ordering of the pairs.  For example,
> 
> ldr x1, [x0, #4]  <--- volatile
> ldr x2, [x0]
> 
> would create a ldp with the volatile load being reordering, if I'm not mistaken.
Hi Chad.
I am really appreciate your review.

As you said, it has a possiblity of reordering.
But I think it's not harmful. After merging, it changed to ldp instruction that has no memory reference information. This means ldp instruction is treated like volatile instruction. 

I will create a static helper function. If you feel something is missing, please tell me.

Thanks.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:1150
@@ +1149,3 @@
+  MachineBasicBlock::iterator Paired;
+  if (isVolatile) {
+    // Only check the next instruction, if this is volatile load.
----------------
mcrosier wrote:
> Please add a few comments about what's going on here.
I will do that.
Thanks.


http://reviews.llvm.org/D14489