[PATCH] D134320: [llvm] Teach whole program devirtualization about relative vtables

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.
Herald added a reviewer: mpaszkowski.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:571
+    }
+  } else if (const auto *CE = dyn_cast<ConstantExpr>(stripCasts(I))) {
+    // If this constant can be reduced to the offset between a function and the
----------------
leonardchan wrote:
> tejohnson wrote:
> > Do we also need to look through DSOLocalEquivalent in this function, as is done in getPointerAtOffset, or why not?
> Not needed here since `IsConstantOffsetFromGlobal` already handles DSOLocalEquivalent to get the offset and global.
This is just stripping `Trunc`, right? You could put this below this line instead of introducing `stripCasts`:
```
if (CE->getOpcode() == Instruction::Trunc)
  CE = CE->getOperand(0);
```


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:579
+      if (IsConstantOffsetFromGlobal(CE->getOperand(0), LHS, LHSOffset, DL) &&
+          IsConstantOffsetFromGlobal(CE->getOperand(1), RHS, RHSOffset, DL) &&
+          RHS == &OrigGV) {
----------------
leonardchan wrote:
> tejohnson wrote:
> > Do we need to do any checking or handling of LHSOffset and RHSOffset?
> In the context of `findFuncPointers`, I don't think so. LHS/RHSOffset just return the byte offset of either LHS/RHS from their respective globals (like via a GEP). `IsConstantOffsetFromGlobal` is just a nifty function for getting the underlying global in these cases. The relevant bits here are confirming RHS refers to the original vtable and we continue to track the function via LHS. 
I think Teresa's concern was that if the offsets don't match this could end up being loaded as `function + 1` or something, which would not be callable. But I suppose we could just say that because calling e.g. `function + 1` is UB we can ignore that possibility and pretend that `function` was being called instead. So we don't need to check the offset. It may be worth adding a comment about that though.


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:140
                                    Constant *TopLevelGlobal) {
+  if (auto *Equiv = dyn_cast<DSOLocalEquivalent>(I))
+    I = Equiv->getGlobalValue();
----------------
I suppose it is the caller who knows whether it is appropriate to strip the `DSOLocalEquivalent`. But it seems that the existing callers should be fine with it so I don't mind adding this here.

It's somewhat suboptimal that this function handles absolute and relative pointers in the same function, but I suppose this is justifiable for the "UB" reasons that also justify ignoring the offset above. It may have been better to split out the "destructuring" part of this function from the pointer-identifying part (which could be done in two functions, one for absolute pointers and another for relative). That would make the semantics clearer at least. But you didn't add the relative pointer support to this function so I don't mind if we don't clean this up for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134320/new/

https://reviews.llvm.org/D134320