[PATCH] D16795: WholeProgramDevirt: introduce.

[Pete Cooper via llvm-commits]

pete added a comment.

Hi Peter

This is great.  Although also quite large, so apologies in advance if it takes a few rounds of review.

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.  That'll greatly reduce the scope of what we have here.

Also, I know Chandler has been passionate about data structure choices in the past.  Luckily he's already CCed here, but i'd double check with him whether things like std::set and std::map are reasonable choices for what you're doing here.

Thanks,
Pete


================
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;
----------------
Is this needed for this patch?  I know its just an optimization, but can it be committed separately?

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:640
@@ -639,1 +639,3 @@
 
+void PassManagerBuilder::addEarlyLTOOptimizationPasses(
+    legacy::PassManagerBase &PM) {
----------------
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.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:10-26
@@ +9,19 @@
+//
+// This pass implements whole program optimization of virtual calls in cases
+// where we know (via bitset information) that the list of callee is fixed. This
+// includes the following:
+// - Single implementation devirtualization: if a virtual call has a single
+//   possible callee, replace all calls with a direct call to that callee.
+// - Virtual constant propagation: if the virtual function's return type is an
+//   integer <=64 bits and all possible callees are readnone, for each class and
+//   each list of constant arguments: evaluate the function, store the return
+//   value alongside the virtual table, and rewrite each virtual call as a load
+//   from the virtual table.
+// - Uniform return value optimization: if the conditions for virtual constant
+//   propagation hold and each function returns the same constant value, replace
+//   each virtual call with that constant.
+// - Unique return value optimization for i1 return values: if the conditions
+//   for virtual constant propagation hold and a single vtable's function
+//   returns 0, or a single vtable's function returns 1, replace each virtual
+//   call with a comparison of the vptr against that vtable's address.
+//
----------------
The tests you've included are very good in terms of ensuring all of this is covered.  However, can you please add negative tests for everything too.  At least one test for each of the main points would be good, for example, a test returning an i128 doesn't get virtual constant propagation.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:112
@@ +111,3 @@
+    // FIXME: see if alignment helps.
+    for (unsigned I = 0;; ++I) {
+      for (auto &&B : Used) {
----------------
Nitpicking: space between the ;;

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:115
@@ +114,3 @@
+        unsigned Byte = 0;
+        while (I + Byte < B.size() && Byte < Size / 8) {
+          if (B[I + Byte])
----------------
Please use () here around the + and / operators to make it clear what precedence you want.

================
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);
----------------
You're comparing pointers here.  Is that safe/intended?

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:281
@@ +280,3 @@
+
+bool WholeProgramDevirt::runOnModule(Module &Mod) {
+  M = &Mod;
----------------
This method is pretty huge.  Can you break it out in to one method for each type of optimization you are doing?  That would make it easier to review each chunk individually.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:304
@@ +303,3 @@
+       I != E;) {
+    auto CI = cast<CallInst>(I->getUser());
+    ++I;
----------------
This is highly unlikely, but there's no guarantee that the user is a CallInst here (or in any of the other similar casts in your patch).

Unfortunately, you could have something like 'call ... (bitcast i1 @llvm.bitset.test to ...)

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:307-310
@@ +306,6 @@
+
+    auto BitSetMDVal = dyn_cast<MetadataAsValue>(CI->getArgOperand(1));
+    if (!BitSetMDVal)
+      report_fatal_error(
+          "Second argument of llvm.bitset.test must be metadata");
+    auto BitSet = BitSetMDVal->getMetadata();
----------------
As you are using an intrinsic here, seems fair to just use a cast<MetadataAsValue> instead of report_fatal_error as either way you'll know if anyone changes the intrinsic types.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:333
@@ +332,3 @@
+    if (CI->use_empty())
+      CI->eraseFromParent();
+  }
----------------
You can use RecursivelyDeleteTriviallyDeadInstructions instead to also delete any dead arguments to the call.


http://reviews.llvm.org/D16795