[PATCH] D14514: [AArch64]Merge narrow zero stores to wider single store

[Jun Bum Lim via llvm-commits]

junbuml added inline comments.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:706-707
@@ +705,4 @@
+    // Change the scaled offset from small to large type.
+    if (!IsUnscaled)
+      OffsetImm /= 2;
+    MIB = BuildMI(*I->getParent(), InsertionPoint, I->getDebugLoc(),
----------------
t.p.northover wrote:
> junbuml wrote:
> > t.p.northover wrote:
> > > What happens here for sequences like
> > > 
> > >     strb wzr, [x0, #1]
> > >     strb wzr, [x0, #2]
> > > 
> > > ? It looks like you might try to produce "strh wzr, [x0, #1]" (using STRHHui) which is invalid. This also applies to the code recently added around line 537.
> > This pass will not merge the input you mentioned above. In findMatchingInsn(), we check if the new scaled wide load can express the scaled narrow inputs in around line 948 below in this patch.
> >  
> > 
> Ah, I see. Could we put some divisibility asserts in here just in case future modifications mess things around?
Sure. I will add assert here.

================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:952
@@ -903,3 +951,3 @@
           // input, bail and keep looking.
           if (!IsUnscaled && alignTo(MinOffset, 2) != MinOffset) {
             trackRegDefsUses(MI, ModifiedRegs, UsedRegs, TRI);
----------------
t.p.northover wrote:
> That hard-coded 2 looks very suspicious to me. Won't permit an incorrect STRHHui -> STRWui transformation?
Since we merge two narrow zero stores to one zero store by doubling the datasize, I think 2 is just right value here for scaled case. For unscaled case, we don't need to have this check. 

Since this patch transforms narrow stores which store only zero, I think STRHHui -> STRWui is also valid. Please let me know if there is any case I missed about the incorrect STRHHui -> STRWui transformation.



http://reviews.llvm.org/D14514