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

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Oct 29 18:29:38 PDT 2019


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:
> tejohnson wrote:
> > 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.
> Is it allowed having weak_odr + linkonce_odr + available_externally in P.VTableVI.getSummaryList()? If yes (I think it should be) than `all but one must be available_externally` is misleading. If not the code itself should be changed.
Yes, that is a valid combination. I have changed the comment and the code. I made the lookup of the first non-available externally into a loop so I can also check for the case of multiple locals vtables with the same guid owing to not enough distinguishing path and same source file name (handled conservatively by returning false), and added a test case.


================
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
----------------
tejohnson wrote:
> 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.
Changed as described above - use a loop so I can also look for the clashing local guid case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69452





More information about the llvm-commits mailing list