[PATCH] D11200: Improve merging of stores from static constructors in GlobalOpt

[Nick Lewycky]

nlewycky added a comment.

This looks nearly ready to land. I've convinced myself that "MergePendingStores" is doing the right thing.


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1974
@@ +1973,3 @@
+
+    for (int i = 2; i < NumOpA && i < NumOpB; i++) {
+      ConstantInt *IndexA = cast<ConstantInt>(A->getOperand(i));
----------------
Add:
  assert(cast<ConstantInt>(A->getOperand(1))->isZero() && "GEP A steps over object");
and another for B? Something to make it clear why int i starts at 2.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2003
@@ +2002,3 @@
+  DenseMap<GlobalVariable *, MutatedGlobal> Globals;
+  typedef DenseMap<GlobalVariable *, MutatedGlobal>::const_iterator const_iterator;
+
----------------
80 columns! I heartily recommend clang-format.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2011-2012
@@ +2010,4 @@
+    if (ConstantExpr *CE = dyn_cast<ConstantExpr>(Ptr)) {
+      if (CE->getOpcode() == Instruction::GetElementPtr) {
+        return cast<GlobalVariable>(CE->getOperand(0));
+      }
----------------
Couldn't CE->getOpcode() == Instruction::BitCast also reach here?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2044
@@ +2043,3 @@
+  if (I == Globals.end()) {
+    auto R = Globals.insert(std::make_pair(GV, MutatedGlobal { GV, nullptr, {} }));
+    assert(R.second && "Global value already in the map?");
----------------
80-col

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2116
@@ +2115,3 @@
+
+    // Move onto the next expression.
+    Pending.erase(It++);
----------------
"onto" -> "on to" (See http://www.oxforddictionaries.com/us/words/onto-or-on-to-american for discussion.)

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2187
@@ +2186,3 @@
+  MG.Initializer = nullptr;
+  assert(MG.Pending.empty() && "Expected pending stores to be empty after merging");
+
----------------
80-col


Repository:
  rL LLVM

http://reviews.llvm.org/D11200