[PATCH] D12116: [AArch64] Improve load/store optimizer to handle LDUR + LDR.

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Chad,

As far as I understand, currently ISelLowering splits the unaligned stores, from which we happen to get STUR and STR, which we can't combine to STP without this patch. With this patch, we'll be able to merge them back, so we'll undo that optimization.

I think that being able to combine STUR/STR to STP is good by itself, but from the patch alone we'll probably get regressions (AFAIR, the biggest one was matmul from LNT testsuite). That's my only concern, but I don't mind landing the patch and addressing this issue later if we actually observe it.

Also, I can't provide a high-quality review of this code, as I'm not very familiar with it, so I'd appreciate other people's feedback on the patch.

Thanks!

Michael


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:455
@@ +454,3 @@
+    PairedOffset =
+        PairedIsUnscaled ? PairedOffset / MemSize : PairedOffset * MemSize;
+  }
----------------
mcrosier wrote:
> mzolotukhin wrote:
> > This looks strange. Is it expected that scaled and unscaled offset differ by `MemSize^2`? (in one case you multiply by `MemSize`, in the other - divide)
> My assumption is that a scaled offset is always the unscaled offset divide by the MemSize and conversely the unscaled offset is always the scaled offset divided by the MemSize; I believe this is valid.
> 
> I.e., 
> Unscaled Offset = Scaled Offset * MemSize;
> Scaled Offset = Unscaled Offset / MemSize;
> 
> What did I miss?  I'm trying to simply the logic so that both offsets are either scaled or unscaled, but not a mix of the two.  I'm guessing you're concerned my conversion is incorrect.
> 
> 
Yep, I was concerned that it was incorrect, but I just read the code incorrectly. Sorry for that!
(I thought that we do `PairedOffset = UnscaledOffset*MemSize` in one case and `PairedOffset = UnscaleOffset/MemSize` in the other).


Repository:
  rL LLVM

http://reviews.llvm.org/D12116