<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">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">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">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">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">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">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">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">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>