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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
