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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D36850#2828461 <https://reviews.llvm.org/D36850#2828461>, @modimo wrote:

> In D36850#2827176 <https://reviews.llvm.org/D36850#2827176>, @tejohnson wrote:
>
>> Sorry for not being responsive. I've been out of office but will be back Monday.
>
> No worries!

Thanks again for working on this! Finally had a chance to go through the whole patch.

>> I skimmed through your notes, thanks for all the stats, it looks like nounwind is a good direction. Regarding linkonce_odr, I would think you should be able to take the union of their attributes, since they should be interchangeable.
>
> Looking into this more my example in `linkonce_functionattrs_comdat.ll` is UB given that it's violating language ODR with 2 functionally different definitions. I think under the langref we're safe to propagate off of any single copy of a linkonce_odr however practically speaking taking the union of all of them may be safer and also handle cases where different attributes appear due to de-refinement.
>
>> There should not be a requirement to use the local copy for linkonce_odr. After propagation, wouldn't their attributes be the same (i.e. regardless of inlining, since the callee attributes should presumably propagate up into the callee)?
>
> In the original approach without taking the union a different copy could propagate "norecurse" to the caller but the local definition could actually recurse. If this is inlined, we'll get a caller with the "norecurse" attribute incorrectly which in reality does recurse. Taking the conservative union fixes this issue.

One possibility is to just look at the copy the linker chose as prevailing. We can pass in the isPrevailing callback (see the call to thinLTOInternalizeAndPromoteInIndex just before your new calls to thinLTOPropagateFunctionAttrs).

I have some other ideas listed in the embedded comments for hopefully making it a bit more efficient, and some other comments/questions.



================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:101
 
+static cl::opt<bool> DisableThinLTOPropagation(
+    "disable-thinlto-funcattrs", cl::Hidden,
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:334
+/// based on linkage information.
+std::vector<FunctionSummary *> getFunctionSummaries(ValueInfo VI) {
+  if (!VI)
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:343
+    if (const AliasSummary *AS = dyn_cast<AliasSummary>(GVS.get()))
+      if (!AS->hasAliasee())
+        continue;
----------------
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).


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:348
+    if (GlobalValue::isLocalLinkage(GVS->linkage())) {
+      Summaries.push_back(GVS.get());
+      break;
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:351
+    } else if (GlobalValue::isExternalLinkage(GVS->linkage())) {
+      if (!Summaries.empty()) {
+        return {};
----------------
What if the other summaries are seen after this external linkage summary in the list?

Presumably these cases would be when we have a strong def that overrides weak definitions. In either case the external linkage symbol would presumably have been prevailing, and we can probably assert on that fact here if we pass in the isPrevailing callback.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:355
+      Summaries.push_back(GVS.get());
+    } else if (GlobalValue::isWeakLinkage(GVS->linkage())) {
+      // Accumulates all the weak linkage summaries
----------------
For weak and the below available externally and linkonce cases, we presumably could:
- pick the first one if ODR
- pick the prevailing copy in all cases
I think? We can pass in the isPrevailing callback (see the calls to thinLTOInternalizeAndPromoteInIndex just before the calls you added to thinLTOPropagateFunctionAttrs).


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:365
+  std::vector<FunctionSummary *> ResolvedSummaries;
+  for (auto S : Summaries) {
+    while (S) {
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:372
+      AliasSummary *AS = dyn_cast<AliasSummary>(S);
+      if (!AS || !AS->hasAliasee())
+        return {};
----------------
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).


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:375
+      S = AS->getBaseObject();
+      if (S == AS)
+        return {};
----------------
I don't understand what would cause this case.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:410
+        std::vector<FunctionSummary *> tmpSummaries =
+            getFunctionSummaries(Callee.first);
+        // Bail if we don't have a FunctionSummary to work with
----------------
It seems like we would do the same thing here many times for a frequently called function. Can we save some info in a lazily built map and reuse it when already computed? I.e. either the result of getFunctionSummaries or even better a bool of whether any of those summaries might recurse.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:422
+
+    bool calleesMightRecurse = std::any_of(
+        calleeSummaries.begin(), calleeSummaries.end(),
----------------
Can this be folded into the above loop, i.e. where it is currently doing the std::for_each and adding to calleeSummaries? Just update calleesMightRecurse there instead of adding to a new set and walking again here.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:428
+      for (const auto &S : callSummaries) {
+        if (!S->fflags().NoRecurse) {
+          auto newFlags = S->fflags();
----------------
Instead of adding the new setFFlags and doing the checking first here, perhaps just add an interface to set the NoRecurse flag on S and call it unconditionally? Eventually setters can be added for other flags as needed.


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

https://reviews.llvm.org/D36850