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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

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

> @tejohnson Indirect calls are not captured in FunctionSummaries in CallGraph or in a flag form saying they exist. Also looks like <https://github.com/llvm/llvm-project/blob/92ce6db/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L372> speculative candidates for ICP do make it on the graph. For this analysis we need to bail out on indirect calls so I think the simplest way is to add a flag indicating the presence of them (In FunFlags?). As for the speculative candidates, it's probably not too big of a deal.

Good point on indirect calls. Rather than add a bit to the summary, can the flags just be set conservatively in any function containing an indirect call when we build the summaries initially? I think that would get the same effect. For speculative devirtualization aka ICP, we will still be left with a fallback indirect call, so it would still need to be treated conservatively. The extra edges added for ICP promotion candidates shouldn't be a problem or affect this.

Note that with class hierarchy analysis we can do better for virtual calls, e.g. when -fwhole-program-vtables is enabled for whole program devirtualization and we have the necessary whole program visibility on vtables. We could potentially integrate WPD decision here. Even if we can't find a single devirtualization target, we can compute the set of all possible targets of virtual calls during the thin link and could use that information to basically merge the attributes from all possible targets. But probably best to leave that as a future refinement as it will take some additional work to get that hooked up. We'd still need to be conservative for virtual calls when we don't have the necessary type info (when -fwhole-program-vtables not enabled or we don't have whole program visibility on the vtable defs), or for non-virtual indirect calls.



================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:383
+  //   occur in a couple of circumstances:
+  //      a. An internal function gets imported due to its caller getting
+  //      imported, it becomes AvailableExternally
----------------
I'm not sure how this case could be happening as we haven't actually done the importing that would create these new available externally copies yet - that happens in the LTO backends, during the thin link we just add them to import lists.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:388
+  //         propagating on its caller.
+  //      b. C++11 [temp.explicit]p10 can generate AvailableExternally
+  //      definitions of explicitly instanced template declarations
----------------
There is no prevailing copy presumably because the prevailing copy is in a native library being linked? I think these cases can be handled conservatively.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:413
+      assert(AS->hasAliasee());
+      FS = dyn_cast<FunctionSummary>(AS->getBaseObject());
+    } else {
----------------
You can just unconditionally do the getBaseObject call on the GVS without casting to AliasSummary. For non-AliasSummary it will just return itself.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:419
+    // Virtual calls are unknown so go conservative
+    if (!FS || FS->getTypeIdInfo())
+      return CachedAttributes[VI];
----------------
I think this checking for virtual calls will only work if -fwhole-program-vtables is enabled for whole program devirtualization or CFI. Otherwise we don't have the type tests that cause this to get populated. This also won't detect non-virtual indirect calls.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:443
+          // we'll keep the last one seen.
+          ODR = FS;
+        } else if (!Prevailing && !ODR) {
----------------
modimo wrote:
> tejohnson wrote:
> > Won't we still have a copy marked prevailing? Wondering if the weak linkage cases can all be merged.
> Yeah, there will still be a copy that's prevailing. Reading through the linkage descriptions again and also those in `FunctionImportGlobalProcessing::getLinkage`:
> 
> 1. I think with External/WeakODR/LinkOnceODR once the prevailing is found use that copy
> 
> 2. For Weak/LinkOnce even with a prevailing copy I don't know if the copy ultimately used will be prevailing. I'm wondering if a native definition could be the victor in which case we just can't propagate off these functions. 
> 
> WDYT about (2)? For C++ at least these don't seem to really exist and testing with Clang self-build I'm not seeing this kick in anywhere. I added a flag to specifically disable this case so it's easy to test out the differences.
Since the linker which invokes this should have been passed all objects to link, bitcode and native, it can do symbol resolution across all of them. So if there is an overriding native strong symbol, it should see that and the bitcode resolution would be non-prevailing and all bitcode copies marked dead (in computeDeadSymbols). So I think the weak any and linkonce any case can take the prevailing copy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36850