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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.


================
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
----------------
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.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:574
+    // original vtable (`Fn - OrigGV`), then we can still find the function if
+    // we know the function.
+    if (CE->getOpcode() == Instruction::Sub) {
----------------
tejohnson wrote:
> I don't completely understand what is meant by "find the function if we know the function".
Sorry, I don't even know what I typed. It should instead say something like: "If this constant can be reduced to the offset between a function and a global, then we know this is a valid virtual function if the RHS is the original vtable we're scanning through."


================
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) {
----------------
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. 


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1386
       // Jump tables are only profitable if the retpoline mitigation is enabled.
+      if (!CB.getParent()) {
+        // When finding devirtualizable calls, it's possible to find the same
----------------
tejohnson wrote:
> What provoked needing this change, and do we (or why not) need a similar check for all the other WPD optimizations that process the CallSites?
Hmm I distinctly remember running into a segfault in `CB.getCaller()` because this CB no longer had a parent, but I can't seem to reproduce this anymore in a local build, so I guess it can be removed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134320