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

[Nick Lewycky]

nlewycky added a subscriber: nlewycky.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1968
@@ +1967,3 @@
+
+// Sort stores to globals from most significant to least signifcant. The key to
+// the map is either the global variable itself (value clobbers the entire
----------------
Typo, "least signifcant" -> "least significant".

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1968
@@ +1967,3 @@
+
+// Sort stores to globals from most significant to least signifcant. The key to
+// the map is either the global variable itself (value clobbers the entire
----------------
nlewycky wrote:
> Typo, "least signifcant" -> "least significant".
You don't define what significant means here? It appears to mean lowest start offset first then largest range?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:1969-1970
@@ +1968,4 @@
+// Sort stores to globals from most significant to least signifcant. The key to
+// the map is either the global variable itself (value clobbers the entire
+// initializer), or a GEP expression (value merges into initializer).
+struct MutatedComparator {
----------------
Is the map allowed to contain both? What about overlapping GEP expressions (you can achieve this by having one GEP with more indices than another)?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2015-2020
@@ +2014,8 @@
+
+  GlobalVariable *GetGlobalForAddr(Constant *Addr) {
+    GlobalVariable *GV = dyn_cast<GlobalVariable>(Addr);
+    if (!GV) {
+      ConstantExpr *CE = dyn_cast<ConstantExpr>(Addr);
+      if (CE) {
+        GV = cast<GlobalVariable>(CE->getOperand(0));
+      }
----------------
This will assert on the fully generalized possibilities of an llvm::Constant (for instance a ConstantExpr bitcast of an llvm::Function).

That couldn't happen when isSimpleEnoughPointerToCommit(Addr) is true, but I don't see any reason why, by construction, that is guaranteed to happen?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2038
@@ +2037,3 @@
+      auto R = Globals.insert(std::make_pair(GV, MutatedGlobal { GV, {} }));
+      assert(R.second);
+      I = R.first;
----------------
Please include a short message explaining why this invariant exists. For example:
  assert(R.second && "Adding store to a global that isn't mutated?");

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2212-2213
@@ +2211,4 @@
+
+  // If the address isn't a GEP expression, completely clobber the initializer.
+  if (!Addr) {
+    Constant *Val = It->second;
----------------
What if the address is a bitcast? If Addr is a bitcast of i32* @GV to i8*, then it would only clobber the first byte of the initializer.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2237-2238
@@ -2130,3 +2236,4 @@
     NumElts = ATy->getNumElements();
-  else
-    NumElts = InitTy->getVectorNumElements();
+  } else if (STy) {
+    NumElts = STy->getNumElements();
+  } else if (VTy) {
----------------
This is a functionality change (unless it's dead code). Could you please add a test for it? Better, can it be hoisted out of this change and submitted in advance?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2242
@@ +2241,3 @@
+  } else {
+    assert(false && "Unexpected initializer type");
+  }
----------------
Use:
  llvm_unreachable("Unexpected initializer type");
here. See http://llvm.org/docs/CodingStandards.html#assert-liberally .

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2289
@@ +2288,3 @@
+  Constant *Init = MergeStoresInto(MG.GV->getInitializer(), Start, End, 0, 2);
+  assert(Start == End);
+  MG.GV->setInitializer(Init);
----------------
Also that MG.Stores.empty() ?


Repository:
  rL LLVM

http://reviews.llvm.org/D11200