[PATCH] D36850: [ThinLTO] Add noRecurse and noUnwind thinlink function attribute propagation

[Di Mo via Phabricator via cfe-commits]

modimo added inline comments.


================
Comment at: clang/test/CodeGen/thinlto-funcattr-prop.ll:16
+
+; CHECK: ^2 = gv: (guid: 13959900437860518209, summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 1, canAutoHide: 0), insts: 2, calls: ((callee: ^3)))))
+; CHECK: ^3 = gv: (guid: 14959766916849974397, summaries: (function: (module: ^1, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0, noUnwind: 1, mayThrow: 0, hasUnknownCall: 0))))
----------------
tejohnson wrote:
> I believe this corresponds to call_extern - why aren't we getting noRecurse and noUnwind propagated here?
> 
> (also, suggest adding a comment above each of these summaries as to what function name they correspond to)
Tracing through llvm-lto2 the index is written out by `CombinedIndexHook` before the rest of thinlink including attribute propagation takes place. The attributes do end up successfully getting propagated, I'll add a check for that in the `*1.promote.bc` which shows the outcome of the attributes being propagated.

Good idea, added the function name that correspond to each summary. 


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:480
+        if (!CalleeSummary->fflags().NoUnwind ||
+            CalleeSummary->fflags().MayThrow)
+          InferredFlags.NoUnwind = false;
----------------
tejohnson wrote:
> Please make sure one of the may throw propagation tests would fail without this fix (i.e. when it was checking the caller's maythrow setting).
Thinking more on why this didn't manifest strange behavior: because of the BU order of call-graph traversal any callee that has mayThrow will have its inferred noUnwind set to false above. Checking again in the caller is redundant because the noUnwind property of the callee will be determined by its value of noUnwind only. I think removing this check completely makes sense.

I can think of a scenario where there are mayThrow instructions but the function is still marked noUnwind (noexcept function with a throw in it) but in that case it is safe to propagate upwards because any exception will fail to escape this callee and so checking mayThrow would actually be a pessimization. I added a case in funcattrs-prop-maythrow.ll to illustrate this.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:1110
+    const auto &GV = DefinedGlobals.find(F.getGUID());
+    if (GV == DefinedGlobals.end())
+      return;
----------------
tejohnson wrote:
> Can this be merged with updateLinkage so we only do the DefinedGlobals lookup once per symbol?
Sure, merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36850