[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: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:
> junbuml wrote:
> > 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.
> > 
> STRHHui -> STRWui is fine, but I think (can't test, the patch no longer applies cleanly to trunk) the following would go wrong:
> 
>     strh wzr, [x0, #2]
>     strh wzr, [x0, #4]
> 
> In this case MinOffset == 2, alignTo(MinOffset, 2) == 2, but "str wzr [x0, #2]" isn't encodable.
Sorry, I will rebase the patch soon.

In IR, above your scaled strh case will be looked like : 
  STRHHui %WZR, %X0, 1
  STRHHui %WZR, %X0, 2
Therefore, MinOffset == 1 and alignTo(MinOffset, 2) != 2, so we will not allow this transformation.



http://reviews.llvm.org/D14514