[PATCH] D144209: [ThinLTO/WPD] Handle function alias in vtable correctly

[Teresa Johnson via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:398-400
+    // Be conservative if a non-function has the same GUID (which is rare).
+    else
+      return false;
----------------
mingmingl wrote:
> For my understanding, is this conservative change orthogonal with function aliases in vtable (since `auto *FS = dyn_cast<FunctionSummary>(Summary->getBaseObject())` should be true for an alias)?
It's related, but not strictly necessary for this case now that we get the base object of the summary a couple lines above. Without either of these fixes, we would incorrectly return true from this function for an alias.


================
Comment at: llvm/test/ThinLTO/X86/devirt_function_alias.ll:26
+; RUN:   -r=%t1.o,_ZN1B1mEi,px \
+; RUN:   2>&1 | FileCheck %s --implicit-check-not evirtualized --allow-empty
+; RUN: llvm-dis %t2.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
----------------
mingmingl wrote:
> 
It was on purpose because I was trying to match both "devirtualized" and "Devirtualized" (there are multiple messages on success, one is capitalized and the other isn't. I made this more explicit in the pattern.


================
Comment at: llvm/test/ThinLTO/X86/devirt_function_alias.ll:47
+; RUN:   -r=%t3.o,_ZN1B1mEi, \
+; RUN:   2>&1 | FileCheck %s --implicit-check-not evirtualized --allow-empty
+; RUN: llvm-dis %t4.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
----------------
mingmingl wrote:
> 
see above


================
Comment at: llvm/test/ThinLTO/X86/devirt_function_alias.ll:62
+; RUN:   -r=%t5.o,_ZN1B1mEi,px \
+; RUN:   2>&1 | FileCheck %s --implicit-check-not evirtualized --allow-empty
+; RUN: llvm-dis %t6.0.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
----------------
mingmingl wrote:
> 
see above


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144209