[llvm] 17c20a6 - [SCEV] Avoid unnecessary domination checks (NFC)

[Philip Reames via llvm-commits]

Re-reading this, I realized my tone here is sharper than called for.  
Let me try rewording this.

I'd be surprised if the call to DT.dominates is expensive in this case.  
Do you have any measurements to support this being important compile 
time wise?  I'd prefer to keep the previous structure if not as it seems 
easier to follow to me.  Thoughts?

Philip

On 10/6/21 1:32 PM, Philip Reames wrote:
> Er, do you actually have evidence of this being a problem?  I would be 
> very surprised if calling DT was even *remotely* expensive here, and I 
> would strongly prefer the simpler code.
>
> Philip
>
> On 10/6/21 1:14 PM, Nikita Popov via llvm-commits wrote:
>> Author: Nikita Popov
>> Date: 2021-10-06T22:14:04+02:00
>> New Revision: 17c20a6dfb7c89ac4eb13990308b731263345918
>>
>> URL: 
>> https://github.com/llvm/llvm-project/commit/17c20a6dfb7c89ac4eb13990308b731263345918
>> DIFF: 
>> https://github.com/llvm/llvm-project/commit/17c20a6dfb7c89ac4eb13990308b731263345918.diff
>>
>> LOG: [SCEV] Avoid unnecessary domination checks (NFC)
>>
>> When determining the defining scope, avoid repeatedly querying
>> dominationg against the function entry instruction. This ends up
>> begin a very common case that we can handle more efficiently.
>>
>> Added:
>>
>> Modified:
>>      llvm/include/llvm/Analysis/ScalarEvolution.h
>>      llvm/lib/Analysis/ScalarEvolution.cpp
>>
>> Removed:
>>
>>
>> ################################################################################ 
>>
>> diff  --git a/llvm/include/llvm/Analysis/ScalarEvolution.h 
>> b/llvm/include/llvm/Analysis/ScalarEvolution.h
>> index df1b7c2ad157..a38b1a299163 100644
>> --- a/llvm/include/llvm/Analysis/ScalarEvolution.h
>> +++ b/llvm/include/llvm/Analysis/ScalarEvolution.h
>> @@ -1921,9 +1921,10 @@ class ScalarEvolution {
>>     SCEV::NoWrapFlags getNoWrapFlagsFromUB(const Value *V);
>>       /// Return a scope which provides an upper bound on the 
>> defining scope of
>> -  /// 'S'.  Specifically, return the first instruction in said 
>> bounding scope.
>> +  /// 'S'. Specifically, return the first instruction in said 
>> bounding scope.
>> +  /// Return nullptr if the scope is trivial (function entry).
>>     /// (See scope definition rules associated with flag discussion 
>> above)
>> -  const Instruction *getDefiningScopeBound(const SCEV *S);
>> +  const Instruction *getNonTrivialDefiningScopeBound(const SCEV *S);
>>       /// Return a scope which provides an upper bound on the 
>> defining scope for
>>     /// a SCEV with the operands in Ops.
>>
>> diff  --git a/llvm/lib/Analysis/ScalarEvolution.cpp 
>> b/llvm/lib/Analysis/ScalarEvolution.cpp
>> index b767adac6a59..14f5dd51c406 100644
>> --- a/llvm/lib/Analysis/ScalarEvolution.cpp
>> +++ b/llvm/lib/Analysis/ScalarEvolution.cpp
>> @@ -6585,25 +6585,24 @@ SCEV::NoWrapFlags 
>> ScalarEvolution::getNoWrapFlagsFromUB(const Value *V) {
>>     return isSCEVExprNeverPoison(BinOp) ? Flags : SCEV::FlagAnyWrap;
>>   }
>>   -const Instruction *ScalarEvolution::getDefiningScopeBound(const 
>> SCEV *S) {
>> +const Instruction *
>> +ScalarEvolution::getNonTrivialDefiningScopeBound(const SCEV *S) {
>>     if (auto *AddRec = dyn_cast<SCEVAddRecExpr>(S))
>>       return &*AddRec->getLoop()->getHeader()->begin();
>>     if (auto *U = dyn_cast<SCEVUnknown>(S))
>>       if (auto *I = dyn_cast<Instruction>(U->getValue()))
>>         return I;
>> -  // All SCEVs are bound by the entry to F
>> -  return &*F.getEntryBlock().begin();
>> +  return nullptr;
>>   }
>>     const Instruction *
>>   ScalarEvolution::getDefiningScopeBound(ArrayRef<const SCEV *> Ops) {
>> -  const Instruction *Bound = &*F.getEntryBlock().begin();
>> -  for (auto *S : Ops) {
>> -    auto *DefI = getDefiningScopeBound(S);
>> -    if (DT.dominates(Bound, DefI))
>> -      Bound = DefI;
>> -  }
>> -  return Bound;
>> +  const Instruction *Bound = nullptr;
>> +  for (auto *S : Ops)
>> +    if (auto *DefI = getNonTrivialDefiningScopeBound(S))
>> +      if (!Bound || DT.dominates(Bound, DefI))
>> +        Bound = DefI;
>> +  return Bound ? Bound : &*F.getEntryBlock().begin();
>>   }
>>
>>
>>          _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits