<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Oct 6, 2021 at 10:32 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.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">Er, do you actually have evidence of this being a problem?  I would be <br>
very surprised if calling DT was even *remotely* expensive here, and I <br>
would strongly prefer the simpler code.<br>
<br>
Philip<br></blockquote><div><br></div><div>Yes, this does recover the minor regression that calling dominates() here introduced.</div><div><br></div><div>Also, I consider this the simpler code. I didn't like how D111191 couples implementation details, by having to hardcode the nodes supported by getDefiningScopeBound() in the caller as well. Now you can always call it and use a non-null result.<br></div><div><br></div><div>Regards,</div><div>Nikita<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On 10/6/21 1:14 PM, Nikita Popov via llvm-commits wrote:<br>
> Author: Nikita Popov<br>
> Date: 2021-10-06T22:14:04+02:00<br>
> New Revision: 17c20a6dfb7c89ac4eb13990308b731263345918<br>
><br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/17c20a6dfb7c89ac4eb13990308b731263345918" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/17c20a6dfb7c89ac4eb13990308b731263345918</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/17c20a6dfb7c89ac4eb13990308b731263345918.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/17c20a6dfb7c89ac4eb13990308b731263345918.diff</a><br>
><br>
> LOG: [SCEV] Avoid unnecessary domination checks (NFC)<br>
><br>
> When determining the defining scope, avoid repeatedly querying<br>
> dominationg against the function entry instruction. This ends up<br>
> begin a very common case that we can handle more efficiently.<br>
><br>
> Added:<br>
>      <br>
><br>
> Modified:<br>
>      llvm/include/llvm/Analysis/ScalarEvolution.h<br>
>      llvm/lib/Analysis/ScalarEvolution.cpp<br>
><br>
> Removed:<br>
>      <br>
><br>
><br>
> ################################################################################<br>
> diff  --git a/llvm/include/llvm/Analysis/ScalarEvolution.h b/llvm/include/llvm/Analysis/ScalarEvolution.h<br>
> index df1b7c2ad157..a38b1a299163 100644<br>
> --- a/llvm/include/llvm/Analysis/ScalarEvolution.h<br>
> +++ b/llvm/include/llvm/Analysis/ScalarEvolution.h<br>
> @@ -1921,9 +1921,10 @@ class ScalarEvolution {<br>
>     SCEV::NoWrapFlags getNoWrapFlagsFromUB(const Value *V);<br>
>   <br>
>     /// Return a scope which provides an upper bound on the defining scope of<br>
> -  /// 'S'.  Specifically, return the first instruction in said bounding scope.<br>
> +  /// 'S'. Specifically, return the first instruction in said bounding scope.<br>
> +  /// Return nullptr if the scope is trivial (function entry).<br>
>     /// (See scope definition rules associated with flag discussion above)<br>
> -  const Instruction *getDefiningScopeBound(const SCEV *S);<br>
> +  const Instruction *getNonTrivialDefiningScopeBound(const SCEV *S);<br>
>   <br>
>     /// Return a scope which provides an upper bound on the defining scope for<br>
>     /// a SCEV with the operands in Ops.<br>
><br>
> diff  --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp<br>
> index b767adac6a59..14f5dd51c406 100644<br>
> --- a/llvm/lib/Analysis/ScalarEvolution.cpp<br>
> +++ b/llvm/lib/Analysis/ScalarEvolution.cpp<br>
> @@ -6585,25 +6585,24 @@ SCEV::NoWrapFlags ScalarEvolution::getNoWrapFlagsFromUB(const Value *V) {<br>
>     return isSCEVExprNeverPoison(BinOp) ? Flags : SCEV::FlagAnyWrap;<br>
>   }<br>
>   <br>
> -const Instruction *ScalarEvolution::getDefiningScopeBound(const SCEV *S) {<br>
> +const Instruction *<br>
> +ScalarEvolution::getNonTrivialDefiningScopeBound(const SCEV *S) {<br>
>     if (auto *AddRec = dyn_cast<SCEVAddRecExpr>(S))<br>
>       return &*AddRec->getLoop()->getHeader()->begin();<br>
>     if (auto *U = dyn_cast<SCEVUnknown>(S))<br>
>       if (auto *I = dyn_cast<Instruction>(U->getValue()))<br>
>         return I;<br>
> -  // All SCEVs are bound by the entry to F<br>
> -  return &*F.getEntryBlock().begin();<br>
> +  return nullptr;<br>
>   }<br>
>   <br>
>   const Instruction *<br>
>   ScalarEvolution::getDefiningScopeBound(ArrayRef<const SCEV *> Ops) {<br>
> -  const Instruction *Bound = &*F.getEntryBlock().begin();<br>
> -  for (auto *S : Ops) {<br>
> -    auto *DefI = getDefiningScopeBound(S);<br>
> -    if (DT.dominates(Bound, DefI))<br>
> -      Bound = DefI;<br>
> -  }<br>
> -  return Bound;<br>
> +  const Instruction *Bound = nullptr;<br>
> +  for (auto *S : Ops)<br>
> +    if (auto *DefI = getNonTrivialDefiningScopeBound(S))<br>
> +      if (!Bound || DT.dominates(Bound, DefI))<br>
> +        Bound = DefI;<br>
> +  return Bound ? Bound : &*F.getEntryBlock().begin();<br>
>   }<br>
>   <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>
</blockquote></div></div>