[PATCH] D43690: [ThinLTO] Keep available_externally symbols live

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp:550
+    // EliminateAvailableExternally pass and setting them to not-live breaks
+    // downstreams users of liveness information (PR36483).
+    if (isPrevailing(VI.getGUID()) == PrevailingType::No) {
----------------
mehdi_amini wrote:
> mehdi_amini wrote:
> > vlad.tsyrklevich wrote:
> > > mehdi_amini wrote:
> > > > This explanation is not clear to me. I understand that this is fixing a bug (crash), but it isn't clear as of *why* this is the right fix?
> > > > I'm not sure why would the simple fact that a function is defined somewhere as "available eternally" would be enough to make them "live" if we don't have any reference anywhere to make them live for another reason?
> > > I believe the logic here is actually different. https://reviews.llvm.org/D42107 changed the live root analysis to mark non-prevailing symbols as not live even if they are reachable. I changed the logic here to continue to mark available_externally symbols live only if they're reachable.
> > OK, if so then I misunderstood what's going on, but I'm also a bit lost with why are these symbols special with respect to liveness. I.e. why is it OK to make other "non-prevailing" symbols not live but not these ones? The comment just says "to not-live breaks downstreams users of liveness information", which falls a bit short for me to really understand the "why" here.
> The "mark non-prevailing symbols as not live even if they are reachable" isn't clear to me either, but at this point I'm probably not up-to-date on the subtleties of "prevailing" vs "live" concept (Apple does not use "prevailing" as a concept)
Prevailing is used to describe which definition of a symbol (e.g. with interposable linkage where multiple definition are available) is to be included in the final link. If you look at D42107 it's fixing https://bugs.llvm.org/show_bug.cgi?id=35938: when LTO marked the ELF symbol as prevailing, previously the ThinLTO symbol would still be marked live and internalized. D42107 was meant to address that; however, available_externally symbols are never prevailing--they are never meant to be included in the final build output. This means that even if one is called it would not have been marked live, and downstream passes like LowerTypeTests would not know that that symbol is actually used and emit jumptable entries for that symbol. We add special handling for available_externally because as far as I'm aware it should be the only linkage type that could hit this edge case (it's not prevailing but references to it are live, they are just replaced with references to a DSO instead.)


Repository:
  rL LLVM

https://reviews.llvm.org/D43690