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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Few follow ups below.



================
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))))
----------------
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)


================
Comment at: llvm/include/llvm/Transforms/IPO/FunctionImport.h:224
+                             const GVSummaryMapTy &DefinedGlobals,
+                             bool PropagateAttrs = false);
 
----------------
Suggest either removing default since you are always passing this argument, or default it to true and stop passing it in the places where it is true (since generally we want this to be true except in a few ThinLTOCodeGenerator.cpp locations that are testing specific things that don't involve propagation). Some preference for the former option (removing default), to make sure any new callers that get added think through the appropriate value.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:480
+        if (!CalleeSummary->fflags().NoUnwind ||
+            CalleeSummary->fflags().MayThrow)
+          InferredFlags.NoUnwind = false;
----------------
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).


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


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop.ll:6
+
+;; ThinLTO Function attribute propagation uses the prevailing symbol to propagate attributes to its callers. Interposable (linkonce and weak) linkages are fair game given we know the prevailing copy 
+;; will be used in the final binary.
----------------
Nit,  line length


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36850