[PATCH] D18890: [AArch64] add SSA Load Store optimization pass

[Jongwon Lee via llvm-commits]

JongwonLee added inline comments.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:71
@@ +70,3 @@
+// Scaling factor for unscaled load or store.
+static int getMemScale(MachineInstr *MI) {
+  switch (MI->getOpcode()) {
----------------
rengolin wrote:
> JongwonLee wrote:
> > rengolin wrote:
> > > This functionality seems to match with TII's isUnscaledLdSt(), maybe it could be merged there?
> > TII's isUnscaledLdSt() just returns boolean type accroding to opcode. Actually, getMemScale() is called dependently on the result of isUnscaledLdSt(). This usage pattern is also found in AArch64LoadStoreOptimizer.cpp. 
> To me, it seems like isUnscaledLdSt should be just:
> 
>     return (getMemScale() == 1);
But there exists unscaled ld/st with mem scale that is not 1.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:168
@@ +167,3 @@
+  int FirstMIOffset = getLdStOffsetOp(FirstMI).getImm();
+  int FirstMIOffsetStride = FirstMIIsUnscaled ? getMemScale(FirstMI) : 1;
+
----------------
rengolin wrote:
> JongwonLee wrote:
> > rengolin wrote:
> > > You have added the ops with scale 1, and an assert to make sure you're passing the right opcode. Shouldn't you just call getMemScale here and use it as an opcode check, and set the flag with (Stride == 1)?
> > I cannot catch your point. Could you show me a code sample you want?
>     int FirstMIOffsetStride = getMemScale(FirstMI);
>     bool FirstMIIsUnscaled = (FirstMIOffsetStride == 1);
> 
> or, if you do the transformation I mentioned above:
> 
>     int FirstMIOffsetStride = TII->getMemScale(FirstMI);
>     bool FirstMIIsUnscaled = TII->isUnscaledLdSt(FirstMI)
> 
> Inliners could common up both calls to getMemScale.
Thanks for the example. But mem-scale is not always the same as offset-stride. The stride of scaled ld/st is 1, and the stride of unscaled is mem-scale.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:380
@@ +379,3 @@
+      //  dst reg of second load = src reg of second store
+      if (getLdStRegOp(consecutive_loads[0]).getReg() !=
+              getLdStRegOp(consecutive_stores[0]).getReg() ||
----------------
mcrosier wrote:
> Has this been clang-formatted?
Is this format is wrong? What is required in the clang-format?

================
Comment at: lib/Target/AArch64/AArch64TargetMachine.cpp:114
@@ +113,3 @@
+                                   " optimization pass in SSA form"),
+                          cl::init(true), cl::Hidden);
+
----------------
rengolin wrote:
> Maybe not leave it on by default as a first approach?
I fixed it to be off by default.

================
Comment at: test/CodeGen/AArch64/ssa-ldst-opt.ll:1
@@ +1,2 @@
+; RUN: llc -march=aarch64 -verify-machineinstrs -asm-verbose=false -o - %s | FileCheck %s
+
----------------
rengolin wrote:
> It's good to add the 'aarch64-ssa-load-store-opt' flag, even if it ends up enabled by default for two reasons:
> 
> 1. It'll document that this is what the test is about, so people can easily find in the code what the change was.
> 
> 2. If it ever ends up disabled by default, the tests will not break.
'aarch64-ssa-load-store-opt' is disabled by default.
The explanation about this test is described.



http://reviews.llvm.org/D18890