<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:48 AM, Arthur Eubanks
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAPW48srM3jYkgGH7iXqCSTnaPkcLtdwbtTq=009VwuxVv_xEHQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">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.
<div>The legacy PM had PruneEH which ran the same logic of
looking at blocks for noreturn calls.</div>
<div><br>
</div>
<div><br>
</div>
<div>The following fails if you remove handling of
noreturn calls from my patch.</div>
<div><br>
</div>
<div><font face="monospace">; RUN: opt < %s
-passes='function(simplifycfg),cgscc(function-attrs,function(simplifycfg))'
-S | not grep "ret i32"<br>
<br>
declare void @noreturn() noreturn<br>
<br>
define i32 @caller() {<br>
call void @noreturn()<br>
ret i32 17<br>
}<br>
<br>
define i32 @caller2() {<br>
%T = call i32 @caller()<br>
ret i32 %T<br>
}<br>
<br>
define i32 @caller3() {<br>
%T = call i32 @caller2()<br>
ret i32 %T<br>
}</font></div>
<div><font face="monospace"><br>
</font></div>
<div><font face="arial, sans-serif">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.</font></div>
</div>
</blockquote>
<p><font face="arial, sans-serif">So, you're argument comes down to
the fact the FunctionAttrs works bottom up and thus is able to
infer attributes through many layers of callees at once? <br>
</font></p>
<p><font face="arial, sans-serif">I buy that. Would you mind adding
a comment to the code to explain why this needs to be here?</font></p>
<p><font face="arial, sans-serif">Philip<br>
</font></p>
<blockquote type="cite"
cite="mid:CAPW48srM3jYkgGH7iXqCSTnaPkcLtdwbtTq=009VwuxVv_xEHQ@mail.gmail.com"><br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Thu, Jan 7, 2021 at 9:28 AM
Nikita Popov <<a href="mailto:nikita.ppv@gmail.com"
target="_blank" moz-do-not-send="true">nikita.ppv@gmail.com</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">
<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"
target="_blank" 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><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>
</div>
</blockquote>
</body>
</html>