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

[Jongwon Lee via llvm-commits]

JongwonLee marked 6 inline comments as done.
JongwonLee added a comment.

I  have answered some of questions. Remaining questions will be answered later.


================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:1
@@ +1,2 @@
+//===--- AArch64ExynosOpt.cpp --- Suppress store pair formation ---===//
+//
----------------
mcrosier wrote:
> ExynosOpt? :)
Thanks. It should be AArch64SSALoadStoreOptimizer.cpp.

================
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:
> 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. 

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:168
@@ +167,3 @@
+  int FirstMIOffset = getLdStOffsetOp(FirstMI).getImm();
+  int FirstMIOffsetStride = FirstMIIsUnscaled ? getMemScale(FirstMI) : 1;
+
----------------
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?

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:286
@@ +285,3 @@
+    MachineBasicBlock::iterator &I, std::set<MachineInstr *> &stores,
+    std::set<MachineInstr *> &loads) {
+  MachineBasicBlock::iterator E = I->getParent()->end();
----------------
rengolin wrote:
> The names of the variables 'loads' and 'stores' is not very explanatory. Maybe call them deletedLoads/Stores? old?
I agree with your opinion. Thanks. I think Loads/StoresWillBeDeleted is appropriate.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:324
@@ +323,3 @@
+
+  std::vector<MachineInstr *> consecutive_stores;
+  std::vector<MachineInstr *> consecutive_loads;
----------------
mcrosier wrote:
> Why not SmallVector?
Thanks. I will fix it.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:407
@@ +406,3 @@
+  bool Modified = false;
+  // 1) Find consecutive two 32-bit loads and consecutive two 32-bit stores that
+  //    write the values of the consecutive 32-bit loads. Transform the
----------------
rengolin wrote:
> This comment is in the wrong place. It should be at a higher level, where people can actually read it before going through the code.
I referenced this format from AArch64LoadStoreOptimizer.cpp. I think there will be more chances to optimize at this level. So, I think that the explanation is necessary for each case even though this patch starts with just one optimization case. 
Maybe we will have ..

```
2) description for second optimization
...
3) description for third optimization
...
```

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:446
@@ +445,3 @@
+  //     STURXi %vreg2, %vreg1, 148; 148 = 37 * 4
+  std::set<MachineInstr *> loads;
+  std::set<MachineInstr *> stores;
----------------
mcrosier wrote:
> mcrosier wrote:
> > loads -> Loads
> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
> 
> In general, names should be in camel case (e.g. TextFileReader and isLValue()).
It will be changed to LoadsWillBeDeleted.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:447
@@ +446,3 @@
+  std::set<MachineInstr *> loads;
+  std::set<MachineInstr *> stores;
+  for (MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end(); MBBI != E;
----------------
mcrosier wrote:
> stores -> Stores
It will be changed to StoresWillBeDeleted.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:448
@@ +447,3 @@
+  std::set<MachineInstr *> stores;
+  for (MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end(); MBBI != E;
+       ++MBBI) {
----------------
mcrosier wrote:
> range-based loop?
The loop iterates from the first instruction to the end instruction in a basic block. Is this the answer you expect? I don't know the exact meaning of the range-based loop.

================
Comment at: test/CodeGen/AArch64/ldst-opt.ll:1125
@@ -1124,3 +1124,3 @@
   %i = phi i64 [ %dec.i, %for.body], [ %count, %0 ]
-  %gep1 = getelementptr i32, i32* %phi1, i64 -1
+  %gep1 = getelementptr i32, i32* %phi1, i64 -3
   %load1 = load i32, i32* %gep1
----------------
rengolin wrote:
> Can you explain this change? Looks suspicious...
This change is to prevent the optimization of this patch from being applied. This test is for another optimization.


http://reviews.llvm.org/D18890