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