[PATCH] D57203: [ThinLTO] Refine reachability check to fix compile time increase

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:830
+        visit(Index.getValueInfo(AS->getAliaseeGUID()));
+        continue;
+      }
----------------
trentxintong wrote:
> mehdi_amini wrote:
> > trentxintong wrote:
> > > mehdi_amini wrote:
> > > > I'm wondering about the `continue` here, the alias itself won't go through `Summary->setLive(true);` a few lines below, does it matter? 
> > > > (I don't know if aliases can't have refs() as well, but it'll also skip the loop)
> > > My understanding is that the alias has already been set alive when it reaches here, i.e. only live GVs are pushed into the Worklist.
> > > 
> > > Thank you for improving this @tejohnson.
> > Then what is ` Summary->setLive(true);` line for? It only applies to Summary coming from the worklist as well? It just seems strange that this is specific to aliases.
> @mehdi_amini  Good question !. AFAIK, we need this setLive(true) in case of an alias (before this patch), because if we do not, we could make the alias alive but not the aliasee. And the thinlto backend will drop dead symbols. However, it does not seem this Summary->setLive(true) is necessary here because aliasee would have been set alive in the visit() function.
We don't need to do the refs and calls iterations for an Alias, since it doesn't have any. The call to visit() that I added will ensure that the aliasee is marked live and added to the worklist if it is not already live, so the aliasee will come through this code eventually and have its refs/calls.

In terms of why I left in the setLive for non-aliases, I am not completely sure it is needed but didn't want to modify the non-alias handling in this patch. Also I had a slight concern that the code at line 760 which adds the initial live values to the worklist does not result in all copies being marked live (if say one was marked live in the per-module index during the summary analysis phase), and without marking all copies live here we would run into the same issue that motivated the patch I'm revising here (since visit() will return if any copy marked live). We can consider modifying this in a future patch but I just wanted to modify the alias handling here to be more efficient and not touch the other handling.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57203