[PATCH] D93946: [FuncAttrs] Infer noreturn

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D93946#2492918 <https://reviews.llvm.org/D93946#2492918>, @sanwou01 wrote:

> Hi folks, I'm afraid an 8% regression in h264ref (SPEC INT 2006) bisects to this patch. This in on AArch64 with -flto on a Neoverse-N1 based system, but I wouldn't be surprised if this showed up elsewhere too.
>
> How do people feel about reverting the patch while we investigate the regression? To start with, I'll try to find out why this has caused the drop in performance: it doesn't seem immediately obvious to me.

I really can't see this change causing performance regressions. It only infers more precise function attributes. Could you do some initial investigation or come up with a repro? Maybe an optimization remark that doesn't fire after this change?



================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:1400
+
+    bool CanReturn = false;
+    // The function can return if any basic blocks terminating with a ReturnInst
----------------
madhur13490 wrote:
> aeubanks wrote:
> > madhur13490 wrote:
> > > I think you can actually avoid this variable. You can declare "FoundNoReturnCall" outside the loops and define it in each iteration of the outer loop. If (!FoundNoReturnCall) in outer loop then break and if (FoundNoReturnCall) outside the loop then F->setDoesNotReturn(). CanReturn is acting as an accumulator here but if you declare FoundNoReturnCall outside the loop then it can act for the same purpose.
> > I'm not following, could you post the code?
> > This is basically an `all(any())` where the `all` means "every basic block must" and the `any` means "any instruction in the block doesn't return". I believe you need two variables for `all(any())`.
> Ah! I misremembered `noreturn` as `any(any)`.  But I guess you can still do it with one variable if you have `unsetDoesNotReturn` in llvm::Function. Something like below,
> 
> ```
>  bool Changed = false;
>     F->setDoesNotReturn();
>     for (BasicBlock &BB : *F) {
>       if (!isa<ReturnInst>(BB.getTerminator()))
>         continue;
>       bool BasicBlockCanReachReturn = true;
>       for (Instruction &I : BB) {
>         if (instructionDoesNotReturn(I)) {
>           BasicBlockCanReachReturn = false;
>           break;
>         }
>       }
>       if (BasicBlockCanReachReturn) {
>         F->unsetDoesNotReturn();
>         Changed = true;
>         break;
>       }
>     }
> 
> ```
> The method is not currently present but I see no reason for ignoring it. I see removeFnAttr being use to one can add such method. More generally, there can be methods like changeNoReturnAttrib(bool) which changes the state to the input bool; if changeNoReturnAttrib(true) then it adds the attribute, if changeNoReturnAttrib(false) it remove the attribute - of course after checking redundancy.
> 
> However,  I think we can continue for now.
> 
To clarify, when you say
> However, I think we can continue for now.
do you mean the current patch looks good or that 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93946/new/

https://reviews.llvm.org/D93946