[llvm] r252871 - Revert "Revert "[FunctionAttrs] Identify norecurse functions""
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 12 08:38:28 PST 2015
On Thu, Nov 12, 2015 at 8:32 AM, James Molloy <james at jamesmolloy.co.uk>
wrote:
> Hi David,
>
> 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.
>
Ah, OK, no worries.
> 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.
>
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.
> 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.
>
& 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.
- Dave
>
> Cheers,
>
> James
>
> On Thu, 12 Nov 2015 at 16:24 David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Thu, Nov 12, 2015 at 8:17 AM, James Molloy <james at jamesmolloy.co.uk>
>> wrote:
>>
>>> Hi David,
>>>
>>> Understood. I'll make my un-revert commit messages more detailed in
>>> future.
>>>
>>> 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.
>>>
>>
>> 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)
>>
>>
>>>
>>> James
>>>
>>> On Thu, 12 Nov 2015 at 16:14 David Blaikie via llvm-commits <
>>> llvm-commits at lists.llvm.org> wrote:
>>>
>>>> On Thu, Nov 12, 2015 at 2:55 AM, James Molloy via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Author: jamesm
>>>>> Date: Thu Nov 12 04:55:20 2015
>>>>> New Revision: 252871
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=252871&view=rev
>>>>> Log:
>>>>> Revert "Revert "[FunctionAttrs] Identify norecurse functions""
>>>>>
>>>>> This reapplies this patch, with test fixes.
>>>>>
>>>>
>>>> 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.
>>>>
>>>>
>>>>>
>>>>> Added:
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll
>>>>> Modified:
>>>>> llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
>>>>> llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll
>>>>>
>>>>> Modified: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=252871&r1=252870&r2=252871&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp (original)
>>>>> +++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Thu Nov 12
>>>>> 04:55:20 2015
>>>>> @@ -50,6 +50,7 @@ STATISTIC(NumReadOnlyArg, "Number of arg
>>>>> STATISTIC(NumNoAlias, "Number of function returns marked noalias");
>>>>> STATISTIC(NumNonNullReturn, "Number of function returns marked
>>>>> nonnull");
>>>>> STATISTIC(NumAnnotated, "Number of attributes added to library
>>>>> functions");
>>>>> +STATISTIC(NumNoRecurse, "Number of functions marked as norecurse");
>>>>>
>>>>> namespace {
>>>>> typedef SmallSetVector<Function *, 8> SCCNodeSet;
>>>>> @@ -63,7 +64,12 @@ struct FunctionAttrs : public CallGraphS
>>>>> }
>>>>>
>>>>> bool runOnSCC(CallGraphSCC &SCC) override;
>>>>> -
>>>>> + bool doInitialization(CallGraph &CG) override {
>>>>> + Revisit.clear();
>>>>> + return false;
>>>>> + }
>>>>> + bool doFinalization(CallGraph &CG) override;
>>>>> +
>>>>> void getAnalysisUsage(AnalysisUsage &AU) const override {
>>>>> AU.setPreservesCFG();
>>>>> AU.addRequired<AssumptionCacheTracker>();
>>>>> @@ -73,6 +79,7 @@ struct FunctionAttrs : public CallGraphS
>>>>>
>>>>> private:
>>>>> TargetLibraryInfo *TLI;
>>>>> + SmallVector<WeakVH,16> Revisit;
>>>>> };
>>>>> }
>>>>>
>>>>> @@ -976,6 +983,14 @@ static void setDoesNotAlias(Function &F,
>>>>> }
>>>>> }
>>>>>
>>>>> +static bool setDoesNotRecurse(Function &F) {
>>>>> + if (F.doesNotRecurse())
>>>>> + return false;
>>>>> + F.setDoesNotRecurse();
>>>>> + ++NumNoRecurse;
>>>>> + return true;
>>>>> +}
>>>>> +
>>>>> /// Analyze the name and prototype of the given function and set any
>>>>> applicable
>>>>> /// attributes.
>>>>> ///
>>>>> @@ -1779,6 +1794,55 @@ static bool inferPrototypeAttributes(Fun
>>>>> return true;
>>>>> }
>>>>>
>>>>> +static bool addNoRecurseAttrs(const CallGraphSCC &SCC,
>>>>> + SmallVectorImpl<WeakVH> &Revisit) {
>>>>> + // Try and identify functions that do not recurse.
>>>>> +
>>>>> + // If the SCC contains multiple nodes we know for sure there is
>>>>> recursion.
>>>>> + if (!SCC.isSingular())
>>>>> + return false;
>>>>> +
>>>>> + const CallGraphNode *CGN = *SCC.begin();
>>>>> + Function *F = CGN->getFunction();
>>>>> + if (!F || F->isDeclaration() || F->doesNotRecurse())
>>>>> + return false;
>>>>> +
>>>>> + // If all of the calls in F are identifiable and are to norecurse
>>>>> functions, F
>>>>> + // is norecurse. This check also detects self-recursion as F is not
>>>>> currently
>>>>> + // marked norecurse, so any called from F to F will not be marked
>>>>> norecurse.
>>>>> + if (std::all_of(CGN->begin(), CGN->end(),
>>>>> + [](const CallGraphNode::CallRecord &CR) {
>>>>> + Function *F = CR.second->getFunction();
>>>>> + return F && F->doesNotRecurse();
>>>>> + }))
>>>>> + // Function calls a potentially recursive function.
>>>>> + return setDoesNotRecurse(*F);
>>>>> +
>>>>> + // We know that F is not obviously recursive, but we haven't been
>>>>> able to
>>>>> + // prove that it doesn't actually recurse. Add it to the Revisit
>>>>> list to try
>>>>> + // again top-down later.
>>>>> + Revisit.push_back(F);
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> +static bool addNoRecurseAttrsTopDownOnly(Function *F) {
>>>>> + // If F is internal and all uses are in norecurse functions, then F
>>>>> is also
>>>>> + // norecurse.
>>>>> + if (F->doesNotRecurse())
>>>>> + return false;
>>>>> + if (F->hasInternalLinkage()) {
>>>>> + for (auto *U : F->users())
>>>>> + if (auto *I = dyn_cast<Instruction>(U)) {
>>>>> + if (!I->getParent()->getParent()->doesNotRecurse())
>>>>> + return false;
>>>>> + } else {
>>>>> + return false;
>>>>> + }
>>>>> + return setDoesNotRecurse(*F);
>>>>> + }
>>>>> + return false;
>>>>> +}
>>>>> +
>>>>> bool FunctionAttrs::runOnSCC(CallGraphSCC &SCC) {
>>>>> TLI = &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
>>>>> bool Changed = false;
>>>>> @@ -1826,6 +1890,19 @@ bool FunctionAttrs::runOnSCC(CallGraphSC
>>>>> Changed |= addNoAliasAttrs(SCCNodes);
>>>>> Changed |= addNonNullAttrs(SCCNodes, *TLI);
>>>>> }
>>>>> +
>>>>> + Changed |= addNoRecurseAttrs(SCC, Revisit);
>>>>> + return Changed;
>>>>> +}
>>>>>
>>>>> +bool FunctionAttrs::doFinalization(CallGraph &CG) {
>>>>> + bool Changed = false;
>>>>> + // When iterating over SCCs we visit functions in a bottom-up
>>>>> fashion. Some of
>>>>> + // the rules we have for identifying norecurse functions work best
>>>>> with a
>>>>> + // top-down walk, so look again at all the functions we previously
>>>>> marked as
>>>>> + // worth revisiting, in top-down order.
>>>>> + for (auto &F : reverse(Revisit))
>>>>> + if (F)
>>>>> + Changed |=
>>>>> addNoRecurseAttrsTopDownOnly(cast<Function>((Value*)F));
>>>>> return Changed;
>>>>> }
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll?rev=252871&r1=252870&r2=252871&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
>>>>> (original)
>>>>> +++ llvm/trunk/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll
>>>>> Thu Nov 12 04:55:20 2015
>>>>> @@ -30,7 +30,7 @@ define void @test1_yes(i32* %p) nounwind
>>>>> ret void
>>>>> }
>>>>>
>>>>> -; CHECK: define void @test1_no(i32* %p) #1 {
>>>>> +; CHECK: define void @test1_no(i32* %p) #3 {
>>>>> define void @test1_no(i32* %p) nounwind {
>>>>> call void @callee(i32* %p), !tbaa !2
>>>>> ret void
>>>>> @@ -72,9 +72,11 @@ define i32 @test3_no(i8* %p) nounwind {
>>>>> declare void @callee(i32* %p) nounwind
>>>>> declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i32, i1)
>>>>> nounwind
>>>>>
>>>>> -; CHECK: attributes #0 = { nounwind readnone }
>>>>> -; CHECK: attributes #1 = { nounwind }
>>>>> +; CHECK: attributes #0 = { norecurse nounwind readnone }
>>>>> +; CHECK: attributes #1 = { norecurse nounwind }
>>>>> ; CHECK: attributes #2 = { nounwind readonly }
>>>>> +; CHECK: attributes #3 = { nounwind }
>>>>> +; CHECK: attributes #4 = { argmemonly nounwind }
>>>>>
>>>>> ; Root note.
>>>>> !0 = !{ }
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll?rev=252871&r1=252870&r2=252871&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll
>>>>> (original)
>>>>> +++ llvm/trunk/test/Transforms/FunctionAttrs/2008-09-03-ReadNone.ll
>>>>> Thu Nov 12 04:55:20 2015
>>>>> @@ -10,15 +10,16 @@ define i32 @f() {
>>>>> ret i32 %tmp
>>>>> }
>>>>>
>>>>> -; CHECK: define i32 @g() #0
>>>>> +; CHECK: define i32 @g() #1
>>>>> define i32 @g() readonly {
>>>>> ret i32 0
>>>>> }
>>>>>
>>>>> -; CHECK: define i32 @h() #0
>>>>> +; CHECK: define i32 @h() #1
>>>>> define i32 @h() readnone {
>>>>> %tmp = load i32, i32* @x ; <i32> [#uses=1]
>>>>> ret i32 %tmp
>>>>> }
>>>>>
>>>>> ; CHECK: attributes #0 = { readnone }
>>>>> +; CHECK: attributes #1 = { norecurse readnone }
>>>>>
>>>>> Modified:
>>>>> llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll?rev=252871&r1=252870&r2=252871&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll
>>>>> (original)
>>>>> +++ llvm/trunk/test/Transforms/FunctionAttrs/2010-10-30-volatile.ll
>>>>> Thu Nov 12 04:55:20 2015
>>>>> @@ -4,7 +4,9 @@
>>>>> @g = constant i32 1
>>>>>
>>>>> define void @foo() {
>>>>> -; CHECK: void @foo() {
>>>>> +; CHECK: void @foo() #0 {
>>>>> %tmp = load volatile i32, i32* @g
>>>>> ret void
>>>>> }
>>>>> +
>>>>> +; CHECK: attributes #0 = { norecurse }
>>>>>
>>>>> Modified: llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll?rev=252871&r1=252870&r2=252871&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll (original)
>>>>> +++ llvm/trunk/test/Transforms/FunctionAttrs/atomic.ll Thu Nov 12
>>>>> 04:55:20 2015
>>>>> @@ -19,5 +19,5 @@ entry:
>>>>> ret i32 %r
>>>>> }
>>>>>
>>>>> -; CHECK: attributes #0 = { readnone ssp uwtable }
>>>>> -; CHECK: attributes #1 = { ssp uwtable }
>>>>> +; CHECK: attributes #0 = { norecurse readnone ssp uwtable }
>>>>> +; CHECK: attributes #1 = { norecurse ssp uwtable }
>>>>>
>>>>> Added: llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll?rev=252871&view=auto
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll (added)
>>>>> +++ llvm/trunk/test/Transforms/FunctionAttrs/norecurse.ll Thu Nov 12
>>>>> 04:55:20 2015
>>>>> @@ -0,0 +1,57 @@
>>>>> +; RUN: opt < %s -basicaa -functionattrs -S | FileCheck %s
>>>>> +
>>>>> +; CHECK: define i32 @leaf() #0
>>>>> +define i32 @leaf() {
>>>>> + ret i32 1
>>>>> +}
>>>>> +
>>>>> +; CHECK: define i32 @self_rec() #1
>>>>> +define i32 @self_rec() {
>>>>> + %a = call i32 @self_rec()
>>>>> + ret i32 4
>>>>> +}
>>>>> +
>>>>> +; CHECK: define i32 @indirect_rec() #1
>>>>> +define i32 @indirect_rec() {
>>>>> + %a = call i32 @indirect_rec2()
>>>>> + ret i32 %a
>>>>> +}
>>>>> +; CHECK: define i32 @indirect_rec2() #1
>>>>> +define i32 @indirect_rec2() {
>>>>> + %a = call i32 @indirect_rec()
>>>>> + ret i32 %a
>>>>> +}
>>>>> +
>>>>> +; CHECK: define i32 @extern() #1
>>>>> +define i32 @extern() {
>>>>> + %a = call i32 @k()
>>>>> + ret i32 %a
>>>>> +}
>>>>> +declare i32 @k() readnone
>>>>> +
>>>>> +; CHECK: define internal i32 @called_by_norecurse() #0
>>>>> +define internal i32 @called_by_norecurse() {
>>>>> + %a = call i32 @k()
>>>>> + ret i32 %a
>>>>> +}
>>>>> +define void @m() norecurse {
>>>>> + %a = call i32 @called_by_norecurse()
>>>>> + ret void
>>>>> +}
>>>>> +
>>>>> +; CHECK: define internal i32 @called_by_norecurse_indirectly() #0
>>>>> +define internal i32 @called_by_norecurse_indirectly() {
>>>>> + %a = call i32 @k()
>>>>> + ret i32 %a
>>>>> +}
>>>>> +define internal void @o() {
>>>>> + %a = call i32 @called_by_norecurse_indirectly()
>>>>> + ret void
>>>>> +}
>>>>> +define void @p() norecurse {
>>>>> + call void @o()
>>>>> + ret void
>>>>> +}
>>>>> +
>>>>> +; CHECK: attributes #0 = { norecurse readnone }
>>>>> +; CHECK: attributes #1 = { readnone }
>>>>>
>>>>> Modified: llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll?rev=252871&r1=252870&r2=252871&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> --- llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll (original)
>>>>> +++ llvm/trunk/test/Transforms/FunctionAttrs/optnone.ll Thu Nov 12
>>>>> 04:55:20 2015
>>>>> @@ -16,9 +16,11 @@ define void @test_optnone(i8* %p) noinli
>>>>>
>>>>> declare i8 @strlen(i8*) noinline optnone
>>>>> ; CHECK-LABEL: @strlen
>>>>> -; CHECK: (i8*) #1
>>>>> +; CHECK: (i8*) #2
>>>>>
>>>>> ; CHECK-LABEL: attributes #0
>>>>> -; CHECK: = { readnone }
>>>>> +; CHECK: = { norecurse readnone }
>>>>> ; CHECK-LABEL: attributes #1
>>>>> +; CHECK: = { noinline norecurse optnone }
>>>>> +; CHECK-LABEL: attributes #2
>>>>> ; CHECK: = { noinline optnone }
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151112/ccb3ca95/attachment.html>
More information about the llvm-commits
mailing list