[llvm] 8cf1cc5 - [FuncAttrs] Infer noreturn
Arthur Eubanks via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 7 09:48:07 PST 2021
Yes, SimplifyCFG not running before FunctionAttrs is why I had to look at
noreturn calls in blocks. Even running a round of SimplifyCFG before the
CGSCC pipeline doesn't fix all cases.
The legacy PM had PruneEH which ran the same logic of looking at blocks for
noreturn calls.
The following fails if you remove handling of noreturn calls from my patch.
; RUN: opt < %s
-passes='function(simplifycfg),cgscc(function-attrs,function(simplifycfg))'
-S | not grep "ret i32"
declare void @noreturn() noreturn
define i32 @caller() {
call void @noreturn()
ret i32 17
}
define i32 @caller2() {
%T = call i32 @caller()
ret i32 %T
}
define i32 @caller3() {
%T = call i32 @caller2()
ret i32 %T
}
We could run another round of FunctionAttrs after the function
simplification pipeline within the CGSCC pipeline which might generally
help with more precise function attributes and would remove the need to
look for noreturn calls in this patch.
On Thu, Jan 7, 2021 at 9:28 AM Nikita Popov <nikita.ppv at gmail.com> wrote:
> On Thu, Jan 7, 2021 at 6:07 PM Philip Reames via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Artur,
>>
>> I see a serious factoring problem with this change. Specifically,
>> existing passes (instcombine, simplifycfg) should have pruned
>> instructions after noreturn calls leaving only a unreachable
>> terminator. Given that, scanning the terminator block should not be
>> required at all here.
>>
>> Are you sure your motivating test requires handling noreturn calls
>> explicitly? (As opposed to simply checking for a lack of return
>> instructions?)
>>
>> The only solid argument I see for handling no return calls is analysis
>> of SCCs, but your patch doesn't appear to do that, so it's no relevant.
>> Unless you're planning to support that in the near future?
>>
>> At the moment, I am not asking for a revert. Depending on your answer
>> and resulting discussion, I may.
>>
>> Philip
>>
>
> I think the problem here might be that under the NewPM, SimplifyCFG (which
> does the noreturn unreachable fold) does not run before FunctionAttrs. On
> the legacy PM it does.
>
> Nikita
>
>
>> On 1/5/21 1:26 PM, Arthur Eubanks via llvm-commits wrote:
>> > Author: Arthur Eubanks
>> > Date: 2021-01-05T13:25:42-08:00
>> > New Revision: 8cf1cc578d3288f5b9cfe1584e524f8b1517dc97
>> >
>> > URL:
>> https://github.com/llvm/llvm-project/commit/8cf1cc578d3288f5b9cfe1584e524f8b1517dc97
>> > DIFF:
>> https://github.com/llvm/llvm-project/commit/8cf1cc578d3288f5b9cfe1584e524f8b1517dc97.diff
>> >
>> > LOG: [FuncAttrs] Infer noreturn
>> >
>> > A function is noreturn if all blocks terminating with a ReturnInst
>> > contain a call to a noreturn function. Skip looking at naked functions
>> > since there may be asm that returns.
>> >
>> > This can be further refined in the future by checking unreachable blocks
>> > and taking into account recursion. It looks like the attributor pass
>> > does this, but that is not yet enabled by default.
>> >
>> > This seems to help with code size under the new PM since PruneEH does
>> > not run under the new PM, missing opportunities to mark some functions
>> > noreturn, which in turn doesn't allow simplifycfg to clean up dead code.
>> > https://bugs.llvm.org/show_bug.cgi?id=46858.
>> >
>> > Reviewed By: rnk
>> >
>> > Differential Revision: https://reviews.llvm.org/D93946
>> >
>> > Added:
>> > llvm/test/Transforms/FunctionAttrs/noreturn.ll
>> >
>> > Modified:
>> > llvm/lib/Transforms/IPO/FunctionAttrs.cpp
>> > llvm/test/Transforms/PruneEH/simplenoreturntest.ll
>> >
>> > Removed:
>> >
>> >
>> >
>> >
>> ################################################################################
>> > diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
>> b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
>> > index 0d25d5d319e7..62e3e5ad0a52 100644
>> > --- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
>> > +++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
>> > @@ -1388,6 +1388,42 @@ static bool addNoRecurseAttrs(const SCCNodeSet
>> &SCCNodes) {
>> > return true;
>> > }
>> >
>> > +static bool instructionDoesNotReturn(Instruction &I) {
>> > + if (auto *CB = dyn_cast<CallBase>(&I)) {
>> > + Function *Callee = CB->getCalledFunction();
>> > + return Callee && Callee->doesNotReturn();
>> > + }
>> > + return false;
>> > +}
>> > +
>> > +// A basic block can only return if it terminates with a ReturnInst
>> and does not
>> > +// contain calls to noreturn functions.
>> > +static bool basicBlockCanReturn(BasicBlock &BB) {
>> > + if (!isa<ReturnInst>(BB.getTerminator()))
>> > + return false;
>> > + return none_of(BB, instructionDoesNotReturn);
>> > +}
>> > +
>> > +// Set the noreturn function attribute if possible.
>> > +static bool addNoReturnAttrs(const SCCNodeSet &SCCNodes) {
>> > + bool Changed = false;
>> > +
>> > + for (Function *F : SCCNodes) {
>> > + if (!F || !F->hasExactDefinition() ||
>> F->hasFnAttribute(Attribute::Naked) ||
>> > + F->doesNotReturn())
>> > + continue;
>> > +
>> > + // The function can return if any basic blocks can return.
>> > + // FIXME: this doesn't handle recursion or unreachable blocks.
>> > + if (none_of(*F, basicBlockCanReturn)) {
>> > + F->setDoesNotReturn();
>> > + Changed = true;
>> > + }
>> > + }
>> > +
>> > + return Changed;
>> > +}
>> > +
>> > static SCCNodesResult createSCCNodeSet(ArrayRef<Function *>
>> Functions) {
>> > SCCNodesResult Res;
>> > Res.HasUnknownCall = false;
>> > @@ -1431,6 +1467,7 @@ static bool
>> deriveAttrsInPostOrder(ArrayRef<Function *> Functions,
>> > Changed |= addReadAttrs(Nodes.SCCNodes, AARGetter);
>> > Changed |= addArgumentAttrs(Nodes.SCCNodes);
>> > Changed |= inferConvergent(Nodes.SCCNodes);
>> > + Changed |= addNoReturnAttrs(Nodes.SCCNodes);
>> >
>> > // If we have no external nodes participating in the SCC, we can
>> deduce some
>> > // more precise attributes as well.
>> >
>> > diff --git a/llvm/test/Transforms/FunctionAttrs/noreturn.ll
>> b/llvm/test/Transforms/FunctionAttrs/noreturn.ll
>> > new file mode 100644
>> > index 000000000000..acc538d3ccee
>> > --- /dev/null
>> > +++ b/llvm/test/Transforms/FunctionAttrs/noreturn.ll
>> > @@ -0,0 +1,66 @@
>> > +; RUN: opt < %s -passes='function-attrs' -S | FileCheck %s
>> > +
>> > +declare i32 @f()
>> > +
>> > +; CHECK: Function Attrs: noreturn
>> > +; CHECK-NEXT: @noreturn()
>> > +declare i32 @noreturn() noreturn
>> > +
>> > +; CHECK: Function Attrs: noreturn
>> > +; CHECK-NEXT: @caller()
>> > +define i32 @caller() {
>> > + %c = call i32 @noreturn()
>> > + ret i32 %c
>> > +}
>> > +
>> > +; CHECK: Function Attrs: noreturn
>> > +; CHECK-NEXT: @caller2()
>> > +define i32 @caller2() {
>> > + %c = call i32 @caller()
>> > + ret i32 %c
>> > +}
>> > +
>> > +; CHECK: Function Attrs: noreturn
>> > +; CHECK-NEXT: @caller3()
>> > +define i32 @caller3() {
>> > +entry:
>> > + br label %end
>> > +end:
>> > + %c = call i32 @noreturn()
>> > + ret i32 %c
>> > +}
>> > +
>> > +; CHECK-NOT: Function Attrs: {{.*}}noreturn
>> > +; CHECK: define i32 @caller4()
>> > +define i32 @caller4() {
>> > +entry:
>> > + br label %end
>> > +end:
>> > + %c = call i32 @f()
>> > + ret i32 %c
>> > +}
>> > +
>> > +; CHECK-NOT: Function Attrs: {{.*}}noreturn
>> > +; CHECK: @caller5()
>> > +; We currently don't handle unreachable blocks.
>> > +define i32 @caller5() {
>> > +entry:
>> > + %c = call i32 @noreturn()
>> > + ret i32 %c
>> > +unreach:
>> > + %d = call i32 @f()
>> > + ret i32 %d
>> > +}
>> > +
>> > +; CHECK-NOT: Function Attrs: {{.*}}noreturn
>> > +; CHECK: @caller6()
>> > +define i32 @caller6() naked {
>> > + %c = call i32 @noreturn()
>> > + ret i32 %c
>> > +}
>> > +
>> > +; CHECK: Function Attrs: {{.*}}noreturn
>> > +; CHECK-NEXT: @alreadynoreturn()
>> > +define i32 @alreadynoreturn() noreturn {
>> > + unreachable
>> > +}
>> >
>> > diff --git a/llvm/test/Transforms/PruneEH/simplenoreturntest.ll
>> b/llvm/test/Transforms/PruneEH/simplenoreturntest.ll
>> > index 814f8b4a686f..ccb9f5eb6f59 100644
>> > --- a/llvm/test/Transforms/PruneEH/simplenoreturntest.ll
>> > +++ b/llvm/test/Transforms/PruneEH/simplenoreturntest.ll
>> > @@ -1,4 +1,5 @@
>> > ; RUN: opt < %s -prune-eh -S -enable-new-pm=0 | not grep "ret i32"
>> > +; RUN: opt < %s -passes='function-attrs,function(simplifycfg)' -S |
>> not grep "ret i32"
>> >
>> > declare void @noreturn() noreturn
>> >
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://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/20210107/3937edb7/attachment.html>
More information about the llvm-commits
mailing list