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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini 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) {
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D43690