[PATCH] D42612: [GlobalOpt] Improve common case efficiency of static global initializer evaluation

[Gerolf Hoflehner via Phabricator via llvm-commits]

Gerolf added a comment.

Thank you drilling into this! I have a few questions below. Also, could you comment on the time savings you measured for your implementation?

-Gerolf



================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2282
+  }
+
+  // The algorithm below doesn't handle cases like nested structs, so use the
----------------
Wouldn't it be better to have 3 vectors - GVs, SimpleCEs and ComplexCEs?


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2310
+    if (GV != CurrentGV || ForceCommit) {
+      if (CurrentGV || ForceCommit) {
+        assert(CurrentGV && "Expected a GV to commit to!");
----------------
The logic is fishy. When CurrentGV is the nullptr ForceCommit is still pushing through (or hit the assert, of course, when it is present).  What if there is only one element to initialize? Could this result in a null ptr dereference?


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2339
+    ValidateOrCommitCache(GV, false);
+    ConstantInt *CU = cast<ConstantInt>(GEP->getOperand(2));
+    Elts[CU->getZExtValue()] = Val;
----------------
"CI" would be more consistent with "GV" than "CU" 


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2344
+  // will be committed on a new initializer being processed.
+  auto LastPair = std::prev(CEs.end());
+  GlobalVariable *GV = cast<GlobalVariable>(LastPair->first->getOperand(0));
----------------
Isn't LastPair == CurrentGV?


Repository:
  rL LLVM

https://reviews.llvm.org/D42612