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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

Thanks for your patience, finally had a chance to go through everything much more carefully. Looks good, mostly a bunch of small or nitpicky final suggestions. The main comment/question of significance relates to where hasUnknownCall is being set currently.

Patch title and summary need an update.



================
Comment at: clang/test/CodeGen/thinlto-funcattr-prop.ll:14
+
+; RUN: llvm-dis %t/b.bc -o - | FileCheck %s
+
----------------
This is checking the summary generated by opt, not the result of the llvm-lto2 run.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:575
     unsigned AlwaysInline : 1;
+    unsigned NoUnwind : 1;
+    // Indicate if function contains instructions that mayThrow
----------------
No Unwind needs a comment. And probably a note that it will be updated by function attr propagation. Depends on how we want to handle inline asm calls and other cases that currently set this true below (see my comment there).


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:580
+    // If there are calls to unknown targets (e.g. indirect)
+    unsigned hasUnknownCall : 1;
+
----------------
tejohnson wrote:
> modimo wrote:
> > tejohnson wrote:
> > > Now that we have MayThrow, can we avoid a separate hasUnknownCall bool and just conservatively set MayThrow true in that case?
> > hasUnknownCall is used for norecurse and other future flags as well to stop propagation.
> Ah that makes sense.
nit, maybe change this to hasIndirectCall which I think is more specific?


================
Comment at: llvm/include/llvm/LTO/LTO.h:26
 #include "llvm/Support/thread.h"
+#include "llvm/Transforms/IPO/FunctionAttrs.h"
 #include "llvm/Transforms/IPO/FunctionImport.h"
----------------
Is this needed?


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:379
       } else {
+        HasUnknownCall = true;
         // Skip inline assembly calls.
----------------
Should this be moved below the following checks for inline asm and direct calls? (Not sure what the direct calls case is given that we handle direct calls to "known functions" above though).

If it should stay where it is and treat the below cases as unknown, probably should add tests for them.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:342
+// prevailing definitions and linkage types
+static FunctionSummary *calculateDefinitiveAttributes(
+    ValueInfo VI, DenseMap<ValueInfo, FunctionSummary *> &CachedAttributes,
----------------
Suggest renaming calculateDefinitiveAttributes and CachedAttributes to something like calculatePrevailingSummary and CachedPrevailingSummary which are more accurate now.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:484
+        if (!CalleeSummary->fflags().NoUnwind ||
+            CallerSummary->fflags().MayThrow)
+          InferredFlags.NoUnwind = false;
----------------
You've already set InferredFlags.NoUnwind to false above this loop in the case where MayThrow was set on the CallerSummary.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:499
+          ++NumThinLinkNoRecurse;
+          CachedAttributes[V]->setNoRecurse();
+        }
----------------
I think you can remove this and the below setNoUnwind() call on CachedAttributes[V] since presumably this points to one of the function summaries we update in the below loop.


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:531
+
+/// Insert function attributes in the Index back into the \p TheModule
+void llvm::thinLTOInsertFunctionAttrsForModule(
----------------
nit: suggest "Insert propagated function attributes from the Index ..."


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:537
+
+  for (Function &F : TheModule) {
+    const auto &GV = DefinedGlobals.find(F.getGUID());
----------------
Consider consolidating this function with thinLTOResolvePrevailingInModule, to reduce the number of walks of the module and lookups into the DefinedGlobals map.


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop-indirect.ll:3
+; RUN: opt -thinlto-bc %s -thin-link-bitcode-file=%t1.thinlink.bc -o %t1.bc
+; RUN: llvm-lto2 run -disable-thinlto-funcattrs=0 %t1.bc -o %t.o -r %t1.bc,indirect,px -save-temps
+; RUN: llvm-dis -o - %t.o.1.3.import.bc | FileCheck %s
----------------
Have a second version that tests with -thinlto-funcattrs-optimistic-indirect? I don't see a test for that option anywhere. Or maybe just remove that option - is it really needed?


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop-indirect.ll:9
+
+; CHECK-NOT: ; Function Attrs: norecurse nounwind
+; CHECK: define i32 @indirect(i32 ()* nocapture %0) {
----------------
Perhaps this CHECK-NOT should just look for "Function Attrs:" as you do in some other tests below, in case some other attr is ever added to the IR that isn't related to this propagation, which could allow the CHECK-NOT to succeed for the wrong reasons?


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop.ll:8
+;; 1. If external, linkonce_odr or weak_odr. We capture the attributes of the prevailing symbol for propagation.
+;; 2. If linkonce or weak, individual callers may optimize using different copies. However, the prevailing symbol will be what's in the final binary and thus we can propagate based off of that
+; RUN: llvm-lto2 run -disable-thinlto-funcattrs=0 %t/a.bc %t/b.bc %t/c.bc -o %t1 -save-temps \
----------------
Since linkonce and weak are interposable, it isn't really correct to say that individual callers may optimize using different copies (we try to prevent this in the compiler since the are interposable).


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop.ll:29
+; b.ll contains non-recursing copies
+; c.ll contains recursing copies
+declare void @linkonce_may_unwind()
----------------
s/recursing/throwing/ on these 2 comments?


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop.ll:63
+
+; CHECK: define dso_local void @call_linkonce_may_unwind() [[ATTR_PROP:#[0-9]+]]
+define void @call_linkonce_may_unwind() {
----------------
Suggest putting comments above this one and call_weak_may_unwind below to indicate why one gets the nounwind and the other doesn't (i.e. that the thin link command above selects as prevailing the nounwind version of linkonce_may_unwind from b.ll and the may throw version of weak_may_unwind from c.ll)


================
Comment at: llvm/test/ThinLTO/X86/funcattrs-prop.ll:76
+; CHECK-DAG: attributes [[ATTR_PROP]] = { norecurse nounwind }
+; CHECK-DAG: attributes [[ATTR_NORECURSE]] = { norecurse }
+
----------------
For clarity on what this is actually testing, suggest renaming these as ATTR_NOUNWIND and ATTR_MAYTHROW, or something like that (they are both norecurse, so the current name is a little misleading as to what is being checked).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D36850