[PATCH] D36850: [ThinLTO] Add norecurse function attribute propagation

[Di Mo via Phabricator via llvm-commits]

modimo marked 7 inline comments as done.
modimo added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:101
 
+static cl::opt<bool> DisableThinLTOPropagation(
+    "disable-thinlto-funcattrs", cl::Hidden,
----------------
tejohnson wrote:
> Might be good to commit this off by default at first, and enable for the new tests. Then it will be easier to do more extensive testing (correctness, compile time, performance), e.g. for our internal apps.
Sure, flipped.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:334
+/// based on linkage information.
+std::vector<FunctionSummary *> getFunctionSummaries(ValueInfo VI) {
+  if (!VI)
----------------
tejohnson wrote:
> In general there needs to be better comments below for the various cases. I see, this looks to be cloned from the StackSafety version, which unfortunately did not undergo a code review before commit and I missed until now... Some questions below that you'll probably need to investigate.
Agreed, I've added summarizing comments on what we're doing here.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:343
+    if (const AliasSummary *AS = dyn_cast<AliasSummary>(GVS.get()))
+      if (!AS->hasAliasee())
+        continue;
----------------
tejohnson wrote:
> We should never have !hasAliasee() here. That should only be true in a couple special cases which don't apply here (in the backends when reading a partial index file emitted for distributed ThinLTO, or when building summaries when reading llvm assembly).
Makes sense, changed.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:348
+    if (GlobalValue::isLocalLinkage(GVS->linkage())) {
+      Summaries.push_back(GVS.get());
+      break;
----------------
tejohnson wrote:
> If the linkage is local and we have more than one summary for this guid we can probably just quit early - that should be a weird corner case that can be handled conservatively, i.e. by not propagating. Normally we expect that local symbols from different modules will have different guid's and therefore ValueInfos because the guid is computed by prepending the module path. We could have a guid alias if there wasn't any distinguishing path when each file was compiled, but that should be rare and we can just punt.
That makes sense, punting on it.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:365
+  std::vector<FunctionSummary *> ResolvedSummaries;
+  for (auto S : Summaries) {
+    while (S) {
----------------
tejohnson wrote:
> Can this handling be folded into the above loop so that we don't have to walk the list of summaries again? I.e. a lambda called for each summary before adding to the list of summaries.
> 
> Also, I think the whole while loop could be replaced with something like:
> 
> ```
>       if (FunctionSummary *FS = dyn_cast<FunctionSummary>(S->getBaseObject()))
>         ResolvedSummaries.push_back(FS);
>       else
>         return {};
> ```
> 
> See my specific notes below about some of the cases currently being handled.
I think the original intention from the unreviewed code is that peel through indirection layers of AS->AS->FS. I suspect this isn't too common, I can try adding some diagnostic code to see how many indirections we need as part of the patch.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:375
+      S = AS->getBaseObject();
+      if (S == AS)
+        return {};
----------------
tejohnson wrote:
> I don't understand what would cause this case.
This and the rest of the functions are being re-implemented from top-down.


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

https://reviews.llvm.org/D36850