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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In https://reviews.llvm.org/D42612#991476, @Gerolf wrote:

> 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


With this change, the test case I was using completed compiling in about 45 seconds, a significant portion of which was spent in the front-end/elsewhere in the compiler.

I don't have a baseline to compare against since I ran out of patience after 10 minutes or so. However if my math is right, for the 127k element initializer, we were doing a copy of the entire array for each element of the initializer. Bringing it down to linear time from O(n^2) I estimate it would have taken at least 66 days to complete, probably more.



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


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2310
+    if (GV != CurrentGV || ForceCommit) {
+      if (CurrentGV || ForceCommit) {
+        assert(CurrentGV && "Expected a GV to commit to!");
----------------
Gerolf wrote:
> 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?
I don't think so. If there's one element only then CurrentGV will be pointing to that element, but the cache will need committing. At that point, CurrentGV == GV and ForceCommit == true.


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


================
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));
----------------
Gerolf wrote:
> Isn't LastPair == CurrentGV?
Yes I think you're right.


Repository:
  rL LLVM

https://reviews.llvm.org/D42612