<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 11/03/2015 11:34 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0KiD3b7JsUU_cSKADBBHPoj13HrXqqLrRVOq8SiHnVwukg@mail.gmail.com"
type="cite">
<div dir="ltr">Thanks for the review!
<div><br>
<div>I think your analysis is in general correct. This was
definitely an unintended effect of my change. I thought I
had checked all four of the operations, but I had missed a
return in the routine that adds readonly and readnone.</div>
</div>
<div><br>
</div>
<div>However, I can't see any way this changes behavior because
of what the readonly and readnone analysis itself does: it
scans the function bodies, and should make a conservative
choice about any calls to external functions.</div>
</div>
</blockquote>
At the outer scope, the problem is that it's not a conservative
choice. It's an optimistic one. We start off by assuming that none
of the functions access memory and then go looking for a refutation
of that. If we miss a node in the SCC, we could miss that fact. I
agree that the per-function scan would seem to return a conservative
result when a node in the SCC is filtered from the list of nodes in
the SCC.<br>
<br>
<blockquote
cite="mid:CAGCO0KiD3b7JsUU_cSKADBBHPoj13HrXqqLrRVOq8SiHnVwukg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div><br>
</div>
<div>I thought this might break in an SCC containing an optnone
node because we would never scan the body of the optnone
function... But that also means we would never infer any
attributes on it, and so when it was called by some other
function in the SCC, when we scanned *that* function, we would
see a call to a may-write function and not make any
assumptions.</div>
</div>
</blockquote>
I think this is true, but the if so, it's by accident, not by
design. Clarifying this would be good. We're effectively relying
on the SCC list being passed in not actually being an SCC in this
case. That seems fragile. <br>
<blockquote
cite="mid:CAGCO0KiD3b7JsUU_cSKADBBHPoj13HrXqqLrRVOq8SiHnVwukg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div><br>
</div>
<div>So while this wasn't intended (and I plan to change it to
match the original behavior) as far as I can tell this is
benign. Does that make sense to you? I have tried to form a
test case to hit this as well, and I can't produce anything
that is *actually* more interesting than the existing test
cases, and it all seems to remain correct even after this
change.</div>
</div>
</blockquote>
I *think* the current code is benign, but I'm not sure. I've never
really understood when a call graph iterator returns a nullptr.
That would be the case which is problematic if there was one. <br>
<blockquote
cite="mid:CAGCO0KiD3b7JsUU_cSKADBBHPoj13HrXqqLrRVOq8SiHnVwukg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div><br>
</div>
<div><br>
</div>
<div>It is also worth noting that I think this code behaves a
bit differently than I had originally assumed, and my comments
around ExternalNode are, I think, misleading at best. The old
call graph does not actually take the form I had expected in a
particularly subtle but important corner case and so I'm
having to re-evaluate how I understand its behavior. I'll try
to update this code to have better comments once I've fully
re-internalized the model.</div>
</div>
</blockquote>
Can you expand? I'm not sure what you're getting at here.<br>
<blockquote
cite="mid:CAGCO0KiD3b7JsUU_cSKADBBHPoj13HrXqqLrRVOq8SiHnVwukg@mail.gmail.com"
type="cite"><br>
<div class="gmail_quote">
<div dir="ltr">On Tue, Nov 3, 2015 at 6:15 PM Philip Reames via
llvm-commits <<a moz-do-not-send="true"
href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Chandler,<br>
<br>
I noticed the handling in this change for ExternalNode. I
*think* that<br>
the code in question is only unnecessarily conservative for
non-null and<br>
no alias inference, but is incorrect for readonly, readnone,
and<br>
argument attributes.<br>
<br>
In particular, none of these analyzes should be able to make
any<br>
speculative conclusions about an SCC which contains an
external call.<br>
By filtering the external node out of the SCC, I think you've
allowed<br>
them to do so. Am I wrong here?<br>
<br>
Philip<br>
<br>
On 10/29/2015 11:29 AM, Chandler Carruth via llvm-commits
wrote:<br>
> Author: chandlerc<br>
> Date: Thu Oct 29 13:29:15 2015<br>
> New Revision: 251640<br>
><br>
> URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project?rev=251640&view=rev"
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=251640&view=rev</a><br>
> Log:<br>
> [FunctionAttrs] Provide a single SCC node set to all of
the<br>
> transformations in FunctionAttrs rather than building a
new one each<br>
> time.<br>
><br>
> This isn't trivial because there are different heuristics
from different<br>
> passes for exactly what set they want. The primary
difference is whether<br>
> an *overridable* function completely disables the
synthesis of<br>
> attributes. I've modeled this by directly testing for
overridable, and<br>
> using the common set that excludes external and opt-none
functions.<br>
><br>
> This does cause some changes by disabling more
optimizations in the face<br>
> of opt-none. Specifically, we were still optimizing
*calls* to opt-none<br>
> functions based on their attributes, just not the bodies.
It seems<br>
> better to be conservative on both fronts given the
intended semanticas<br>
> here (best effort to not assume or disturb anything).
I've not tried to<br>
> test this change as it seems complex, brittle, and not
important to the<br>
> implicit contract of opt-none. Instead, it seems more
like a choice that<br>
> should be dictated by the simplified implementation and
the change to be<br>
> acceptable differences within the space of opt-none.<br>
><br>
> A big benefit here is that these transformations no
longer rely on the<br>
> legacy pass manager's SCC types, they just work on
generic sets of<br>
> function pointers. This will make it easy to re-use their
logic in the<br>
> new pass manager.<br>
><br>
> I've also made the transforms static functions instead of
members where<br>
> trivial while I was touching the signatures.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp<br>
><br>
> Modified: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp<br>
> URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=251640&r1=251639&r2=251640&view=diff"
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=251640&r1=251639&r2=251640&view=diff</a><br>
>
==============================================================================<br>
> --- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
(original)<br>
> +++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Thu
Oct 29 13:29:15 2015<br>
> @@ -52,6 +52,10 @@ STATISTIC(NumNonNullReturn, "Number of
f<br>
> STATISTIC(NumAnnotated, "Number of attributes added to
library functions");<br>
><br>
> namespace {<br>
> +typedef SmallSetVector<Function *, 8> SCCNodeSet;<br>
> +}<br>
> +<br>
> +namespace {<br>
> struct FunctionAttrs : public CallGraphSCCPass {<br>
> static char ID; // Pass identification, replacement
for typeid<br>
> FunctionAttrs() : CallGraphSCCPass(ID) {<br>
> @@ -70,10 +74,7 @@ struct FunctionAttrs : public
CallGraphS<br>
> private:<br>
> TargetLibraryInfo *TLI;<br>
><br>
> - bool AddReadAttrs(const CallGraphSCC &SCC);<br>
> - bool AddArgumentAttrs(const CallGraphSCC &SCC);<br>
> - bool AddNoAliasAttrs(const CallGraphSCC &SCC);<br>
> - bool AddNonNullAttrs(const CallGraphSCC &SCC);<br>
> + bool AddReadAttrs(const SCCNodeSet &SCCNodes);<br>
> bool annotateLibraryCalls(const CallGraphSCC
&SCC);<br>
> };<br>
> }<br>
> @@ -99,9 +100,8 @@ enum MemoryAccessKind {<br>
> };<br>
> }<br>
><br>
> -static MemoryAccessKind<br>
> -checkFunctionMemoryAccess(Function &F, AAResults
&AAR,<br>
> - const
SmallPtrSetImpl<Function *> &SCCNodes) {<br>
> +static MemoryAccessKind
checkFunctionMemoryAccess(Function &F, AAResults &AAR,<br>
> + const
SCCNodeSet &SCCNodes) {<br>
> FunctionModRefBehavior MRB =
AAR.getModRefBehavior(&F);<br>
> if (MRB == FMRB_DoesNotAccessMemory)<br>
> // Already perfect!<br>
> @@ -205,25 +205,11 @@ checkFunctionMemoryAccess(Function
&F, A<br>
> }<br>
><br>
> /// Deduce readonly/readnone attributes for the SCC.<br>
> -bool FunctionAttrs::AddReadAttrs(const CallGraphSCC
&SCC) {<br>
> - SmallPtrSet<Function *, 8> SCCNodes;<br>
> -<br>
> - // Fill SCCNodes with the elements of the SCC. Used
for quickly<br>
> - // looking up whether a given CallGraphNode is in this
SCC.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I)<br>
> - SCCNodes.insert((*I)->getFunction());<br>
> -<br>
> +bool FunctionAttrs::AddReadAttrs(const SCCNodeSet
&SCCNodes) {<br>
> // Check if any of the functions in the SCC read or
write memory. If they<br>
> // write memory then they can't be marked readnone or
readonly.<br>
> bool ReadsMemory = false;<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> -<br>
> - if (!F ||
F->hasFnAttribute(Attribute::OptimizeNone))<br>
> - // External node or node we don't want to optimize
- assume it may write<br>
> - // memory and give up.<br>
> - return false;<br>
> -<br>
> + for (Function *F : SCCNodes) {<br>
> // We need to manually construct BasicAA directly
in order to disable its<br>
> // use of other function analyses.<br>
> BasicAAResult
BAR(createLegacyPMBasicAAResult(*this, *F));<br>
> @@ -247,9 +233,7 @@ bool
FunctionAttrs::AddReadAttrs(const C<br>
> // Success! Functions in this SCC do not access
memory, or only read memory.<br>
> // Give them the appropriate attribute.<br>
> bool MadeChange = false;<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> -<br>
> + for (Function *F : SCCNodes) {<br>
> if (F->doesNotAccessMemory())<br>
> // Already perfect!<br>
> continue;<br>
> @@ -325,7 +309,7 @@ public:<br>
> /// consider that a capture, instead adding it to the
"Uses" list and<br>
> /// continuing with the analysis.<br>
> struct ArgumentUsesTracker : public CaptureTracker {<br>
> - ArgumentUsesTracker(const SmallPtrSet<Function *,
8> &SCCNodes)<br>
> + ArgumentUsesTracker(const SCCNodeSet &SCCNodes)<br>
> : Captured(false), SCCNodes(SCCNodes) {}<br>
><br>
> void tooManyUses() override { Captured = true; }<br>
> @@ -338,7 +322,8 @@ struct ArgumentUsesTracker : public
Capt<br>
> }<br>
><br>
> Function *F = CS.getCalledFunction();<br>
> - if (!F || !SCCNodes.count(F)) {<br>
> + if (!F || F->isDeclaration() ||
F->mayBeOverridden() ||<br>
> + !SCCNodes.count(F)) {<br>
> Captured = true;<br>
> return true;<br>
> }<br>
> @@ -366,7 +351,7 @@ struct ArgumentUsesTracker : public
Capt<br>
> bool Captured; // True only if certainly captured
(used outside our SCC).<br>
> SmallVector<Argument *, 4> Uses; // Uses within
our SCC.<br>
><br>
> - const SmallPtrSet<Function *, 8> &SCCNodes;<br>
> + const SCCNodeSet &SCCNodes;<br>
> };<br>
> }<br>
><br>
> @@ -501,20 +486,9 @@ determinePointerReadAttrs(Argument
*A,<br>
> }<br>
><br>
> /// Deduce nocapture attributes for the SCC.<br>
> -bool FunctionAttrs::AddArgumentAttrs(const CallGraphSCC
&SCC) {<br>
> +static bool addArgumentAttrs(const SCCNodeSet
&SCCNodes) {<br>
> bool Changed = false;<br>
><br>
> - SmallPtrSet<Function *, 8> SCCNodes;<br>
> -<br>
> - // Fill SCCNodes with the elements of the SCC. Used
for quickly<br>
> - // looking up whether a given CallGraphNode is in this
SCC.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> - if (F && !F->isDeclaration() &&
!F->mayBeOverridden() &&<br>
> - !F->hasFnAttribute(Attribute::OptimizeNone))<br>
> - SCCNodes.insert(F);<br>
> - }<br>
> -<br>
> ArgumentGraph AG;<br>
><br>
> AttrBuilder B;<br>
> @@ -522,14 +496,7 @@ bool
FunctionAttrs::AddArgumentAttrs(con<br>
><br>
> // Check each function in turn, determining which
pointer arguments are not<br>
> // captured.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> -<br>
> - if (!F ||
F->hasFnAttribute(Attribute::OptimizeNone))<br>
> - // External node or function we're trying not to
optimize - only a problem<br>
> - // for arguments that we pass to it.<br>
> - continue;<br>
> -<br>
> + for (Function *F : SCCNodes) {<br>
> // Definitions with weak linkage may be overridden
at linktime with<br>
> // something that captures pointers, so treat them
like declarations.<br>
> if (F->isDeclaration() ||
F->mayBeOverridden())<br>
> @@ -714,8 +681,7 @@ bool
FunctionAttrs::AddArgumentAttrs(con<br>
> ///<br>
> /// A function is "malloc-like" if it returns either
null or a pointer that<br>
> /// doesn't alias any other pointer visible to the
caller.<br>
> -static bool isFunctionMallocLike(Function *F,<br>
> - SmallPtrSet<Function
*, 8> &SCCNodes) {<br>
> +static bool isFunctionMallocLike(Function *F, const
SCCNodeSet &SCCNodes) {<br>
> SmallSetVector<Value *, 8> FlowsToReturn;<br>
> for (Function::iterator I = F->begin(), E =
F->end(); I != E; ++I)<br>
> if (ReturnInst *Ret =
dyn_cast<ReturnInst>(I->getTerminator()))<br>
> @@ -778,23 +744,10 @@ static bool
isFunctionMallocLike(Functio<br>
> }<br>
><br>
> /// Deduce noalias attributes for the SCC.<br>
> -bool FunctionAttrs::AddNoAliasAttrs(const CallGraphSCC
&SCC) {<br>
> - SmallPtrSet<Function *, 8> SCCNodes;<br>
> -<br>
> - // Fill SCCNodes with the elements of the SCC. Used
for quickly<br>
> - // looking up whether a given CallGraphNode is in this
SCC.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I)<br>
> - SCCNodes.insert((*I)->getFunction());<br>
> -<br>
> +static bool addNoAliasAttrs(const SCCNodeSet
&SCCNodes) {<br>
> // Check each function in turn, determining which
functions return noalias<br>
> // pointers.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> -<br>
> - if (!F ||
F->hasFnAttribute(Attribute::OptimizeNone))<br>
> - // External node or node we don't want to optimize
- skip it;<br>
> - return false;<br>
> -<br>
> + for (Function *F : SCCNodes) {<br>
> // Already noalias.<br>
> if (F->doesNotAlias(0))<br>
> continue;<br>
> @@ -814,8 +767,7 @@ bool
FunctionAttrs::AddNoAliasAttrs(cons<br>
> }<br>
><br>
> bool MadeChange = false;<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> + for (Function *F : SCCNodes) {<br>
> if (F->doesNotAlias(0) ||
!F->getReturnType()->isPointerTy())<br>
> continue;<br>
><br>
> @@ -834,7 +786,7 @@ bool
FunctionAttrs::AddNoAliasAttrs(cons<br>
> /// Returns true if it believes the function will not
return a null, and sets<br>
> /// \p Speculative based on whether the returned
conclusion is a speculative<br>
> /// conclusion due to SCC calls.<br>
> -static bool isReturnNonNull(Function *F,
SmallPtrSet<Function *, 8> &SCCNodes,<br>
> +static bool isReturnNonNull(Function *F, const
SCCNodeSet &SCCNodes,<br>
> const TargetLibraryInfo
&TLI, bool &Speculative) {<br>
> assert(F->getReturnType()->isPointerTy()
&&<br>
> "nonnull only meaningful on pointer types");<br>
> @@ -898,14 +850,8 @@ static bool isReturnNonNull(Function
*F,<br>
> }<br>
><br>
> /// Deduce nonnull attributes for the SCC.<br>
> -bool FunctionAttrs::AddNonNullAttrs(const CallGraphSCC
&SCC) {<br>
> - SmallPtrSet<Function *, 8> SCCNodes;<br>
> -<br>
> - // Fill SCCNodes with the elements of the SCC. Used
for quickly<br>
> - // looking up whether a given CallGraphNode is in this
SCC.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I)<br>
> - SCCNodes.insert((*I)->getFunction());<br>
> -<br>
> +static bool addNonNullAttrs(const SCCNodeSet
&SCCNodes,<br>
> + const TargetLibraryInfo
&TLI) {<br>
> // Speculative that all functions in the SCC return
only nonnull<br>
> // pointers. We may refute this as we analyze
functions.<br>
> bool SCCReturnsNonNull = true;<br>
> @@ -914,13 +860,7 @@ bool
FunctionAttrs::AddNonNullAttrs(cons<br>
><br>
> // Check each function in turn, determining which
functions return nonnull<br>
> // pointers.<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> -<br>
> - if (!F ||
F->hasFnAttribute(Attribute::OptimizeNone))<br>
> - // External node or node we don't want to optimize
- skip it;<br>
> - return false;<br>
> -<br>
> + for (Function *F : SCCNodes) {<br>
> // Already nonnull.<br>
> if
(F->getAttributes().hasAttribute(AttributeSet::ReturnIndex,<br>
>
Attribute::NonNull))<br>
> @@ -937,7 +877,7 @@ bool
FunctionAttrs::AddNonNullAttrs(cons<br>
> continue;<br>
><br>
> bool Speculative = false;<br>
> - if (isReturnNonNull(F, SCCNodes, *TLI, Speculative))
{<br>
> + if (isReturnNonNull(F, SCCNodes, TLI, Speculative))
{<br>
> if (!Speculative) {<br>
> // Mark the function eagerly since we may
discover a function<br>
> // which prevents us from speculating about the
entire SCC<br>
> @@ -954,8 +894,7 @@ bool
FunctionAttrs::AddNonNullAttrs(cons<br>
> }<br>
><br>
> if (SCCReturnsNonNull) {<br>
> - for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> - Function *F = (*I)->getFunction();<br>
> + for (Function *F : SCCNodes) {<br>
> if
(F->getAttributes().hasAttribute(AttributeSet::ReturnIndex,<br>
>
Attribute::NonNull) ||<br>
> !F->getReturnType()->isPointerTy())<br>
> @@ -1835,10 +1774,36 @@ bool
FunctionAttrs::annotateLibraryCalls<br>
> bool FunctionAttrs::runOnSCC(CallGraphSCC &SCC) {<br>
> TLI =
&getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();<br>
><br>
> + // Annotate declarations for which we have special
knowledge.<br>
> bool Changed = annotateLibraryCalls(SCC);<br>
> - Changed |= AddReadAttrs(SCC);<br>
> - Changed |= AddArgumentAttrs(SCC);<br>
> - Changed |= AddNoAliasAttrs(SCC);<br>
> - Changed |= AddNonNullAttrs(SCC);<br>
> +<br>
> + // Fill SCCNodes with the elements of the SCC. Used
for quickly looking up<br>
> + // whether a given CallGraphNode is in this SCC. Also
track whether there are<br>
> + // any external or opt-none nodes that will prevent us
from optimizing any<br>
> + // part of the SCC.<br>
> + SCCNodeSet SCCNodes;<br>
> + bool ExternalNode = false;<br>
> + for (CallGraphSCC::iterator I = SCC.begin(), E =
SCC.end(); I != E; ++I) {<br>
> + Function *F = (*I)->getFunction();<br>
> + if (!F ||
F->hasFnAttribute(Attribute::OptimizeNone)) {<br>
> + // External node or function we're trying not to
optimize - we both avoid<br>
> + // transform them and avoid leveraging information
they provide.<br>
> + ExternalNode = true;<br>
> + continue;<br>
> + }<br>
> +<br>
> + SCCNodes.insert(F);<br>
> + }<br>
> +<br>
> + Changed |= AddReadAttrs(SCCNodes);<br>
> + Changed |= addArgumentAttrs(SCCNodes);<br>
> +<br>
> + // If we have no external nodes participating in the
SCC, we can infer some<br>
> + // more precise attributes as well.<br>
> + if (!ExternalNode) {<br>
> + Changed |= addNoAliasAttrs(SCCNodes);<br>
> + Changed |= addNonNullAttrs(SCCNodes, *TLI);<br>
> + }<br>
> +<br>
> return Changed;<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a moz-do-not-send="true"
href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a moz-do-not-send="true"
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a moz-do-not-send="true"
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>