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

[Tim Northover via llvm-commits]

t.p.northover accepted this revision.
t.p.northover added a comment.
This revision is now accepted and ready to land.

OK, I think this is good then. Thanks!


================
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);
----------------
junbuml wrote:
> 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.
> 
Oh, bother, yes. Thanks for explaining it to me. It might be worth renaming MinOffset to something that doesn't suggest it's the actual offset quite so strongly (MinOffsetImm, as above?).

I can do that after this patch has been committed though.


http://reviews.llvm.org/D14514