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

Mon Oct 28 11:07:02 PDT 2019

evgeny777 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(
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69452