[PATCH] D108741: [IR][GlobalDCE] Add ability to mark vtable methods as eligible for VFE and avoid eliminating non-eligible vfunc in VFE in GlobalDCE

[Florian Hahn via Phabricator via llvm-commits]

fhahn added reviewers: rjmccall, dexonsmith.
fhahn added a comment.

Adding a few additional reviewers to reflect the expanded scope of the patch.



================
Comment at: clang/lib/CodeGen/CGVTables.cpp:805
 
+    // Mark the function pointer in the vtable as "vfe eligible".
+    fnPtr = llvm::VFEEligibleValue::get(cast<llvm::GlobalValue>(fnPtr));
----------------
It might be better to explain *why* the function pointer is wrapped (to allow virtual function elimination), rather than just state the fact that it is wrapped (which can be seen in the code directly below)


================
Comment at: llvm/docs/TypeMetadata.rst:289
+1. All virtual function pointers in the vtable must be wrapped in a
+   ``vfe_eligible`` constant. Function pointers without a ``vfe_eligible``
+   marker are not participating in removal of unused function pointers. See
----------------
kubamracek wrote:
> fhahn wrote:
> > IIUC the `must` here seems to be stronger than it needs to be. Pointers in the vtable *can* be wrapped as `vie_eligible` to consider them for virtual function elimination. But if they aren't, it is still a valid vtable, right?
> Rephrased.
Thanks. I think now this could even just be a follow-up paragraph, rather than a list. `vfe_eligible` is not really a rule to be followed, but something that enables additional optimizations.


================
Comment at: llvm/include/llvm/AsmParser/LLParser.h:65
     std::unique_ptr<Constant *[]> ConstantStructElts;
+    bool VFEEligible = false;
 
----------------
Why is this needed? Can this be handled in a similar fashion to how other constants are handled?


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:518
+    auto VE = dyn_cast<VFEEligibleValue>(I->stripPointerCasts());
+    if (VE) Fn = dyn_cast<Function>(VE->getGlobalValue());
+    else Fn = dyn_cast<Function>(I->stripPointerCasts());
----------------
Does this need a new test or is this covered by the existing tests?


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

https://reviews.llvm.org/D108741