[PATCH] D16795: WholeProgramDevirt: introduce.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/Transforms/IPO/WholeProgramDevirt.cpp:331-347
@@ +330,19 @@
+    auto OpGlobal = dyn_cast<GlobalVariable>(OpConst);
+    if (!OpGlobal)
+      continue;
+
+    uint64_t Offset =
+        cast<ConstantInt>(
+            cast<ConstantAsMetadata>(Op->getOperand(2))->getValue())
+            ->getZExtValue();
+
+    VTableBits *&BitsPtr = GVToBits[OpGlobal];
+    if (!BitsPtr) {
+      Bits.emplace_back();
+      Bits.back().GV = OpGlobal;
+      Bits.back().ObjectSize = M.getDataLayout().getTypeAllocSize(
+          OpGlobal->getInitializer()->getType());
+      BitsPtr = &Bits.back();
+    }
+    BitSets[BitSetID].insert({BitsPtr, Offset});
+  }
----------------
pete wrote:
> Sorry, don't know how to reply to this point correctly in Phab, but here goes:
> 
> So this is the current code to add an entry to BitSets[].  This is where I think we could move the following checks, as that way we know that all elements in BitSets have valid GlobalValue's according to these checks.  I don't think thats too bad so long as we can move the checks here.  Obviously if for some reason we had to duplicate the checks then your current code is better.
> 
> These are the checks I'm thinking of:
> 
>  if (!BS.Bits->GV->isConstant())
>      return false;
> 
>    auto Init = dyn_cast<ConstantArray>(BS.Bits->GV->getInitializer());
>    if (!Init)
>      return false;
If an element has an invalid GlobalValue according to those checks, we cannot apply the optimization for that bitset, because it would be an invalid optimization. Consider:

```
@vt1 = global [1 x i8*] [i8* bitcast void()* @vf1 to i8*]
@vt2 = constant [1 x i8*] [i8* bitcast void()* @vf2 to i8*]


void @f(i8* %this) {
  %vtable = load %this
  llvm.assume(llvm.bitset.test(%vtable, !"bitset"))
  ; make a virtual call through %vtable
}

!0 = {!"bitset", @vt1, 0}
!1 = {!"bitset", @vt2, 0}
!llvm.bitsets = !{!0, !1}
```

If we do those checks here, the pass would add only `@vt2` to `BitSets["bitset"]`, and it would later apply single-implementation devirtualization to turn the virtual call in `@f` into a direct call to `@vf2`. But that would not be correct, because it could potentially be a call to `@vf1` (or potentially any other function, if some other code overwrites the pointer in `@vt1`).

Because we expect this case to never arise in practice, the simplest thing to do is to handle it in the loop later.


http://reviews.llvm.org/D16795