[PATCH] D69452: [ThinLTO/WPD] Fix index-based WPD for available_externally vtables

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 2 inline comments as done.
tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:846
+    // linkonce/weak ODR, or all but one must be available_externally.
     assert(P.VTableVI.getSummaryList().size() == 1 ||
+           llvm::count_if(
----------------
evgeny777 wrote:
> The code below doesn't go well along with the comment. For example it allows having multiple linkonce_odr/weak_odr summaries and multiple ones with available_externally linkage. 
That is the intent of both the code and the comment, which might not be written very clearly. The code previously allowed multiple weak/linkonce odr, and now should also allow for multiple available_externally. Hence the comment "all but one must be available_externally" - because we can have multiple available externally but only one external linkage. Does that clarify things, or do you have alternate wording in mind that could be clearer?

Now that I think about it, this code should probably also proactively guard against the type of situation encountered and handled in D28411 and D67322 of same-named locals in same-named files compiled without enough distinguishing path. In that case we should not assert and just be conservatively correct. The easiest way to do that is to simply return false from this routine if that situation is encountered. I will add that handling.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:856
+               }) <= 1);
     const auto *VS = cast<GlobalVarSummary>(P.VTableVI.getSummaryList()[0].get());
+    // If there is more than one summary, and the first isn't some kind of weak
----------------
evgeny777 wrote:
> Can we use single find_if instead of lines 856-871 for the sake of simplicity?
> ```
> auto I = llvm::find_if(
>           P.VTableVI.getSummaryList(),
>           [&](const std::unique_ptr<GlobalValueSummary> &Summary) {
>             return !GlobalValue::isAvailableExternallyLinkage(Summary->linkage());
>           });
> ```
Good idea, will change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69452