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

[Chad Rosier via llvm-commits]

mcrosier added a comment.

Drive by review.. :)


================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:1
@@ +1,2 @@
+//===--- AArch64ExynosOpt.cpp --- Suppress store pair formation ---===//
+//
----------------
ExynosOpt? :)

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:151
@@ +150,3 @@
+      int UnscaledOffset = Offset * getMemScale(MI);
+      if (UnscaledOffset < 256 && UnscaledOffset >= -256)
+        return true;
----------------
return UnscaledOffset < 256 && UnscaledOffset >= -256;

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

================
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() ||
----------------
Has this been clang-formatted?

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:386
@@ +385,3 @@
+
+      for (auto &I : consecutive_stores) {
+        stores.insert(I);
----------------
No need for extra braces.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:390
@@ +389,3 @@
+
+      for (auto &I : consecutive_loads) {
+        loads.insert(I);
----------------
No need for extra braces.

================
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;
----------------
loads -> Loads

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

================
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;
----------------
stores -> Stores

================
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) {
----------------
range-based loop?

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:459
@@ +458,3 @@
+
+      if (tryToWidenLdStInst(MBBI, stores, loads)) {
+        Modified = true;
----------------
No need for extra braces.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:466
@@ +465,3 @@
+
+  for (auto &I : loads) {
+    I->eraseFromParent();
----------------
No need for extra braces.

================
Comment at: lib/Target/AArch64/AArch64SSALoadStoreOptimizer.cpp:470
@@ +469,3 @@
+
+  for (auto &I : stores) {
+    I->eraseFromParent();
----------------
No need for extra braces.


http://reviews.llvm.org/D18890