[PATCH] D16795: WholeProgramDevirt: introduce.

[Peter Collingbourne via llvm-commits]

pcc added a comment.

> Firstly, can you do the evaluator stuff in another patch. I'm ok saying LGTM on that here assuming that you only extracted the code from GlobalOpt.


Done in r259621.


================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:923
@@ -922,3 +922,3 @@
       M->getFunction(Intrinsic::getName(Intrinsic::bitset_test));
-  if (!BitSetTestFunc)
+  if (!BitSetTestFunc || BitSetTestFunc->use_empty())
     return false;
----------------
pete wrote:
> Is this needed for this patch?  I know its just an optimization, but can it be committed separately?
It isn't needed for this patch specifically. I've committed it separately in r259625.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:640
@@ -639,1 +639,3 @@
 
+void PassManagerBuilder::addEarlyLTOOptimizationPasses(
+    legacy::PassManagerBase &PM) {
----------------
pete wrote:
> Before this is enabled in tree by default (I can see from the clang patch that you have an option to control this), we'll need perf testing and sign off from LTO folks.  Mehdi is CCed which is good, but probably also need some other interested folks and some data to go over to justify the GlobalDCE pass as well as the new code in terms of compile/runtime.
As one data point: I timed the GlobalDCE pass running over a full LTO module of Chromium at about 3 seconds on my machine. That's less than 1% of total runtime (about 20-30 mins), so I think it's tolerable. I'll also get numbers for this new pass, but I don't think it should affect code that doesn't use the whole-program-vtables feature.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:112
@@ +111,3 @@
+    // FIXME: see if alignment helps.
+    for (unsigned I = 0;; ++I) {
+      for (auto &&B : Used) {
----------------
pete wrote:
> Nitpicking: space between the ;;
That isn't what clang-format does, and I'd prefer to stick with that formatting where reasonable.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:174
@@ +173,3 @@
+  bool operator<(const VTableSlot &other) const {
+    return BitSetID < other.BitSetID ||
+           (BitSetID == other.BitSetID && ByteOffset < other.ByteOffset);
----------------
pete wrote:
> You're comparing pointers here.  Is that safe/intended?
This was safe before because I was iterating over an associated `std::vector`. But I realized that the code can just use a `MapVector` instead, so I've done that.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:333
@@ +332,3 @@
+    if (CI->use_empty())
+      CI->eraseFromParent();
+  }
----------------
pete wrote:
> You can use RecursivelyDeleteTriviallyDeadInstructions instead to also delete any dead arguments to the call.
It turns out that we can't do this too early because `CI` may be the only user of the vtable. It's probably simplest to let another pass clean up the other arguments later.


http://reviews.llvm.org/D16795