[PATCH] D16795: WholeProgramDevirt: introduce.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:644
@@ +643,3 @@
+  // whole-program devirtualization and bitset lowering.
+  PM.add(createGlobalDCEPass());
+
----------------
joker.eph wrote:
> This is the beginning of the pipeline right? When do we have unused virtual tables emitted by the front-end? I don't expect this to happen.
This happens after the internalizer pass. If a vtable is linked into the program but turns out to be unused from a whole-program perspective, this pass will remove it.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:208-210
@@ +207,5 @@
+    CS->replaceAllUsesWith(New);
+    if (auto II = dyn_cast<InvokeInst>(CS.getInstruction())) {
+      BranchInst::Create(II->getUnwindDest(), CS.getInstruction());
+      II->getUnwindDest()->removePredecessor(II->getParent());
+    }
----------------
pete wrote:
> I don't tend to work with InvokeInst much.  I trust that what you're doing here is correct, but please add a test to cover it just to make sure it keeps being ok in future.
Added a test. Good thing I did, because this code contained a bug, which is now fixed.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:296-298
@@ +295,5 @@
+      if (GEP->hasAllConstantIndices() && VPtr == GEP->getPointerOperand()) {
+        SmallVector<Value *, 8> Indices(GEP->op_begin() + 1, GEP->op_end());
+        uint64_t GEPOffset = M->getDataLayout().getIndexedOffsetInType(
+            GEP->getSourceElementType(), Indices);
+        findLoadCallsAtConstantOffset(BitSet, User, Offset + GEPOffset, VTable);
----------------
pete wrote:
> This is pretty gross (not your fault!  I looked and most other places do exactly the same thing)
> 
> But I did see one that was much nicer.  Its in ConstantFolding.cpp.  Can you make yours similar?  I might go back and fix the others in tree.
> 
> Note, it was getting a range from a vector of indices, but I think it might be possible to do the equivalent with op_begin() + 1 and op_end().
> 
> makeArrayRef((Value * const *)Ops.data() + 1, Ops.size() - 1)));
I don't think this will work because `op_begin()..op_end()` is an iterator of `Use`s, not `Value`s.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:602
@@ +601,3 @@
+        findLoadCallsAtConstantOffset(BitSet, Ptr, 0, CI->getArgOperand(0));
+    }
+
----------------
joker.eph wrote:
> It seems to me that you have to reconstruct information that could have been generated by the front-end, using a representation like the intrinsic I sent by email.
> This would make completely straightforward since you'd have all the uses off-hand.
While this is true, it isn't clear that the lowered complexity here would not be outweighed by added complexity in the rest of the compiler. For example, if we adopt a design similar to the one you proposed,other parts of the compiler (e.g. constant folding) would need to be taught about the new intrinsic, and the information from the frontend would need to be kept in sync with the program semantics. The advantage of reconstructing information from program semantics is that we know that the semantics are most likely correct, or the program wouldn't work.

Before we consider changing the design, I would like to see a concrete design proposal that takes into account existing features such as CFI. I don't think any potential new design should block this from landing though.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:630-631
@@ +629,4 @@
+      auto OpGlobal = dyn_cast<GlobalVariable>(OpConst);
+      if (!OpGlobal)
+        continue;
+
----------------
pete wrote:
> In addition to these checks, it looks like BitSets is only used by tryFindVirtualCallTargets, and the first thing we do in there is check the following:
> 
>   if (!BS.Bits->GV->isConstant())
>       return false;
> 
>     auto Init = dyn_cast<ConstantArray>(BS.Bits->GV->getInitializer());
>     if (!Init)
>       return false;
> 
> Can we move those checks here so that we don't even populate BitSets[] entries unless these properties hold?
If we encounter one of these, we need to disable the optimization for that bitset only. I suppose we could either keep track of which bitsets we need to disable optimization for, or disable optimization for all bitsets if we see one of these (by clearing BitSets and returning), but at that point we're making the code more complicated and/or losing a feature.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:702
@@ +701,3 @@
+    Alias->setVisibility(B.GV->getVisibility());
+    Alias->takeName(B.GV);
+
----------------
pete wrote:
> No idea if this is possible, so feel free to ignore, but if the original VT had an explicit section on it, would either NewGV or Alias here need to inherit that section?  I have no idea what the correct answer would be.
> 
> For safety sake, we could always exclude VTables with explicit sections from this optimization until we have a good answer.
> 
> Either way, please add a test with an explicit section and make sure we either propagate it as required, or don't run this optimization on it.
I don't think this comes up with real vtables, but NewGV would need to inherit the section (aliases can't have explicit sections). Added test.


http://reviews.llvm.org/D16795