<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Nov 12, 2015 at 8:32 AM, James Molloy <span dir="ltr"><<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi David,<div><br></div><div>I should have been more clear. I'm pretty sure I know what happened - I put the new attribute in correctly sorted order in the attribute printer where it wasn't before in a previous version of the patch. What I'm not sure about is how the test as committed ended up out of sync with the functional code. I was doing quite a bit of cherry-picking and rebasing, and I must have screwed up somehow.</div></div></blockquote><div><br></div><div>Ah, OK, no worries.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>I'm ensuring I re-build and re-run all tests when I "git am" into my git-svn tree in future to avoid repeats.<br></div></div></blockquote><div><br></div><div>Fair enough - I don't mind a little bit of buildbot noise (I've certainly caused more than my fair share of it), just don't like uncertainty. Thanks for the explanations.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>That was what caused the (single) LLVM regression test failure. The Clang ones were because I didn't have clang in the tree I was testing with, and they needed updating.<br></div></div></blockquote><div><br></div><div>& in theory, you shuoldn't need clang in your tree when you're developing LLVM unless you actually change the IR, etc. That's not always possible/true, but we try for it. I'll look into those clang cases you updated and see if we can remove their LLVM dependence so this doesn't tickle the next person to play in this area.<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div><br></div><div>Cheers,</div><div><br></div><div>James</div></div><div class="HOEnZb"><div class="h5"><br><div class="gmail_quote"><div dir="ltr">On Thu, 12 Nov 2015 at 16:24 David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Nov 12, 2015 at 8:17 AM, James Molloy <span dir="ltr"><<a href="mailto:james@jamesmolloy.co.uk" target="_blank">james@jamesmolloy.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi David,<div><br></div><div>Understood. I'll make my un-revert commit messages more detailed in future.</div><div><br></div><div>For the record, the problem was that the expected ordering of attribute names when printed had changed (affecting function-attrs.ll). I'm still not quite sure why this happened.</div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Are you investigating that? Working around unknowns is always a bit concerning, though sometimes the right call for sure. (the only concern I'd have here is if it's non-deterministic output order - but it doesn't sound like that's the case)</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span><font color="#888888"><div><br></div><div>James</div></font></span></div><div><div><br><div class="gmail_quote"><div dir="ltr">On Thu, 12 Nov 2015 at 16:14 David Blaikie via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Nov 12, 2015 at 2:55 AM, James Molloy via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: jamesm<br>
Date: Thu Nov 12 04:55:20 2015<br>
New Revision: 252871<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=252871&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=252871&view=rev</a><br>
Log:<br>
Revert "Revert "[FunctionAttrs] Identify norecurse functions""<br>
<br>
This reapplies this patch, with test fixes.<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>It'd be helpful to detail the fixes (& whatever extra testing was involved (which buildbot scenarios failed, etc) in finding them) so that those changes can get extra scrutiny by anyone who's already reviewed the original patch & just wants to check the new changes.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Added:<br>
llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll<br>
Modified:<br>
llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp<br>
llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll<br>
llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll<br>
llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll<br>
llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll<br>
llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=252871&r1=252870&r2=252871&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=252871&r1=252870&r2=252871&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Thu Nov 12 04:55:20 2015<br>
@@ -50,6 +50,7 @@ STATISTIC(NumReadOnlyArg, "Number of arg<br>
STATISTIC(NumNoAlias, "Number of function returns marked noalias");<br>
STATISTIC(NumNonNullReturn, "Number of function returns marked nonnull");<br>
STATISTIC(NumAnnotated, "Number of attributes added to library functions");<br>
+STATISTIC(NumNoRecurse, "Number of functions marked as norecurse");<br>
<br>
namespace {<br>
typedef SmallSetVector<Function *, 8> SCCNodeSet;<br>
@@ -63,7 +64,12 @@ struct FunctionAttrs : public CallGraphS<br>
}<br>
<br>
bool runOnSCC(CallGraphSCC &SCC) override;<br>
-<br>
+ bool doInitialization(CallGraph &CG) override {<br>
+ Revisit.clear();<br>
+ return false;<br>
+ }<br>
+ bool doFinalization(CallGraph &CG) override;<br>
+<br>
void getAnalysisUsage(AnalysisUsage &AU) const override {<br>
AU.setPreservesCFG();<br>
AU.addRequired<AssumptionCacheTracker>();<br>
@@ -73,6 +79,7 @@ struct FunctionAttrs : public CallGraphS<br>
<br>
private:<br>
TargetLibraryInfo *TLI;<br>
+ SmallVector<WeakVH,16> Revisit;<br>
};<br>
}<br>
<br>
@@ -976,6 +983,14 @@ static void setDoesNotAlias(Function &F,<br>
}<br>
}<br>
<br>
+static bool setDoesNotRecurse(Function &F) {<br>
+ if (F.doesNotRecurse())<br>
+ return false;<br>
+ F.setDoesNotRecurse();<br>
+ ++NumNoRecurse;<br>
+ return true;<br>
+}<br>
+<br>
/// Analyze the name and prototype of the given function and set any applicable<br>
/// attributes.<br>
///<br>
@@ -1779,6 +1794,55 @@ static bool inferPrototypeAttributes(Fun<br>
return true;<br>
}<br>
<br>
+static bool addNoRecurseAttrs(const CallGraphSCC &SCC,<br>
+ SmallVectorImpl<WeakVH> &Revisit) {<br>
+ // Try and identify functions that do not recurse.<br>
+<br>
+ // If the SCC contains multiple nodes we know for sure there is recursion.<br>
+ if (!SCC.isSingular())<br>
+ return false;<br>
+<br>
+ const CallGraphNode *CGN = *SCC.begin();<br>
+ Function *F = CGN->getFunction();<br>
+ if (!F || F->isDeclaration() || F->doesNotRecurse())<br>
+ return false;<br>
+<br>
+ // If all of the calls in F are identifiable and are to norecurse functions, F<br>
+ // is norecurse. This check also detects self-recursion as F is not currently<br>
+ // marked norecurse, so any called from F to F will not be marked norecurse.<br>
+ if (std::all_of(CGN->begin(), CGN->end(),<br>
+ [](const CallGraphNode::CallRecord &CR) {<br>
+ Function *F = CR.second->getFunction();<br>
+ return F && F->doesNotRecurse();<br>
+ }))<br>
+ // Function calls a potentially recursive function.<br>
+ return setDoesNotRecurse(*F);<br>
+<br>
+ // We know that F is not obviously recursive, but we haven't been able to<br>
+ // prove that it doesn't actually recurse. Add it to the Revisit list to try<br>
+ // again top-down later.<br>
+ Revisit.push_back(F);<br>
+ return false;<br>
+}<br>
+<br>
+static bool addNoRecurseAttrsTopDownOnly(Function *F) {<br>
+ // If F is internal and all uses are in norecurse functions, then F is also<br>
+ // norecurse.<br>
+ if (F->doesNotRecurse())<br>
+ return false;<br>
+ if (F->hasInternalLinkage()) {<br>
+ for (auto *U : F->users())<br>
+ if (auto *I = dyn_cast<Instruction>(U)) {<br>
+ if (!I->getParent()->getParent()->doesNotRecurse())<br>
+ return false;<br>
+ } else {<br>
+ return false;<br>
+ }<br>
+ return setDoesNotRecurse(*F);<br>
+ }<br>
+ return false;<br>
+}<br>
+<br>
bool FunctionAttrs::runOnSCC(CallGraphSCC &SCC) {<br>
TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();<br>
bool Changed = false;<br>
@@ -1826,6 +1890,19 @@ bool FunctionAttrs::runOnSCC(CallGraphSC<br>
Changed |= addNoAliasAttrs(SCCNodes);<br>
Changed |= addNonNullAttrs(SCCNodes, *TLI);<br>
}<br>
+<br>
+ Changed |= addNoRecurseAttrs(SCC, Revisit);<br>
+ return Changed;<br>
+}<br>
<br>
+bool FunctionAttrs::doFinalization(CallGraph &CG) {<br>
+ bool Changed = false;<br>
+ // When iterating over SCCs we visit functions in a bottom-up fashion. Some of<br>
+ // the rules we have for identifying norecurse functions work best with a<br>
+ // top-down walk, so look again at all the functions we previously marked as<br>
+ // worth revisiting, in top-down order.<br>
+ for (auto &F : reverse(Revisit))<br>
+ if (F)<br>
+ Changed |= addNoRecurseAttrsTopDownOnly(cast<Function>((Value*)F));<br>
return Changed;<br>
}<br>
<br>
Modified: llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll?rev=252871&r1=252870&r2=252871&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll?rev=252871&r1=252870&r2=252871&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll (original)<br>
+++ llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll Thu Nov 12 04:55:20 2015<br>
@@ -30,7 +30,7 @@ define void @test1_yes(i32* %p) nounwind<br>
ret void<br>
}<br>
<br>
-; CHECK: define void @test1_no(i32* %p) #1 {<br>
+; CHECK: define void @test1_no(i32* %p) #3 {<br>
define void @test1_no(i32* %p) nounwind {<br>
call void @callee(i32* %p), !tbaa !2<br>
ret void<br>
@@ -72,9 +72,11 @@ define i32 @test3_no(i8* %p) nounwind {<br>
declare void @callee(i32* %p) nounwind<br>
declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i32, i1) nounwind<br>
<br>
-; CHECK: attributes #0 = { nounwind readnone }<br>
-; CHECK: attributes #1 = { nounwind }<br>
+; CHECK: attributes #0 = { norecurse nounwind readnone }<br>
+; CHECK: attributes #1 = { norecurse nounwind }<br>
; CHECK: attributes #2 = { nounwind readonly }<br>
+; CHECK: attributes #3 = { nounwind }<br>
+; CHECK: attributes #4 = { argmemonly nounwind }<br>
<br>
; Root note.<br>
!0 = !{ }<br>
<br>
Modified: llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll?rev=252871&r1=252870&r2=252871&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll?rev=252871&r1=252870&r2=252871&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll (original)<br>
+++ llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll Thu Nov 12 04:55:20 2015<br>
@@ -10,15 +10,16 @@ define i32 @f() {<br>
ret i32 %tmp<br>
}<br>
<br>
-; CHECK: define i32 @g() #0<br>
+; CHECK: define i32 @g() #1<br>
define i32 @g() readonly {<br>
ret i32 0<br>
}<br>
<br>
-; CHECK: define i32 @h() #0<br>
+; CHECK: define i32 @h() #1<br>
define i32 @h() readnone {<br>
%tmp = load i32, i32* @x ; <i32> [#uses=1]<br>
ret i32 %tmp<br>
}<br>
<br>
; CHECK: attributes #0 = { readnone }<br>
+; CHECK: attributes #1 = { norecurse readnone }<br>
<br>
Modified: llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll?rev=252871&r1=252870&r2=252871&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll?rev=252871&r1=252870&r2=252871&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll (original)<br>
+++ llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll Thu Nov 12 04:55:20 2015<br>
@@ -4,7 +4,9 @@<br>
@g = constant i32 1<br>
<br>
define void @foo() {<br>
-; CHECK: void @foo() {<br>
+; CHECK: void @foo() #0 {<br>
%tmp = load volatile i32, i32* @g<br>
ret void<br>
}<br>
+<br>
+; CHECK: attributes #0 = { norecurse }<br>
<br>
Modified: llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll?rev=252871&r1=252870&r2=252871&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll?rev=252871&r1=252870&r2=252871&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll (original)<br>
+++ llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll Thu Nov 12 04:55:20 2015<br>
@@ -19,5 +19,5 @@ entry:<br>
ret i32 %r<br>
}<br>
<br>
-; CHECK: attributes #0 = { readnone ssp uwtable }<br>
-; CHECK: attributes #1 = { ssp uwtable }<br>
+; CHECK: attributes #0 = { norecurse readnone ssp uwtable }<br>
+; CHECK: attributes #1 = { norecurse ssp uwtable }<br>
<br>
Added: llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll?rev=252871&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll?rev=252871&view=auto</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll (added)<br>
+++ llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll Thu Nov 12 04:55:20 2015<br>
@@ -0,0 +1,57 @@<br>
+; RUN: opt < %s -basicaa -functionattrs -S | FileCheck %s<br>
+<br>
+; CHECK: define i32 @leaf() #0<br>
+define i32 @leaf() {<br>
+ ret i32 1<br>
+}<br>
+<br>
+; CHECK: define i32 @self_rec() #1<br>
+define i32 @self_rec() {<br>
+ %a = call i32 @self_rec()<br>
+ ret i32 4<br>
+}<br>
+<br>
+; CHECK: define i32 @indirect_rec() #1<br>
+define i32 @indirect_rec() {<br>
+ %a = call i32 @indirect_rec2()<br>
+ ret i32 %a<br>
+}<br>
+; CHECK: define i32 @indirect_rec2() #1<br>
+define i32 @indirect_rec2() {<br>
+ %a = call i32 @indirect_rec()<br>
+ ret i32 %a<br>
+}<br>
+<br>
+; CHECK: define i32 @extern() #1<br>
+define i32 @extern() {<br>
+ %a = call i32 @k()<br>
+ ret i32 %a<br>
+}<br>
+declare i32 @k() readnone<br>
+<br>
+; CHECK: define internal i32 @called_by_norecurse() #0<br>
+define internal i32 @called_by_norecurse() {<br>
+ %a = call i32 @k()<br>
+ ret i32 %a<br>
+}<br>
+define void @m() norecurse {<br>
+ %a = call i32 @called_by_norecurse()<br>
+ ret void<br>
+}<br>
+<br>
+; CHECK: define internal i32 @called_by_norecurse_indirectly() #0<br>
+define internal i32 @called_by_norecurse_indirectly() {<br>
+ %a = call i32 @k()<br>
+ ret i32 %a<br>
+}<br>
+define internal void @o() {<br>
+ %a = call i32 @called_by_norecurse_indirectly()<br>
+ ret void<br>
+}<br>
+define void @p() norecurse {<br>
+ call void @o()<br>
+ ret void<br>
+}<br>
+<br>
+; CHECK: attributes #0 = { norecurse readnone }<br>
+; CHECK: attributes #1 = { readnone }<br>
<br>
Modified: llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll?rev=252871&r1=252870&r2=252871&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll?rev=252871&r1=252870&r2=252871&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll (original)<br>
+++ llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll Thu Nov 12 04:55:20 2015<br>
@@ -16,9 +16,11 @@ define void @test_optnone(i8* %p) noinli<br>
<br>
declare i8 @strlen(i8*) noinline optnone<br>
; CHECK-LABEL: @strlen<br>
-; CHECK: (i8*) #1<br>
+; CHECK: (i8*) #2<br>
<br>
; CHECK-LABEL: attributes #0<br>
-; CHECK: = { readnone }<br>
+; CHECK: = { norecurse readnone }<br>
; CHECK-LABEL: attributes #1<br>
+; CHECK: = { noinline norecurse optnone }<br>
+; CHECK-LABEL: attributes #2<br>
; CHECK: = { noinline optnone }<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a 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></div></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a 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>
</div></div></blockquote></div></div></div></blockquote></div>
</div></div></blockquote></div><br></div></div>