[SCEV] Reduce the number of invocation to non trivial getExact function

[skatkov]

Summary

The invocation of getExact in ScalarEvolution::getBackedgeTakenInfo is used
only for getting statistic and for assert. And even if statistics is disable and code
related to it will be eliminated the invocation to getExact itself will not be eliminated
because it may have side-effects like creation of new SCEVs.

So do invocation only when we collect statistics or executes asserts.

[mkazantsev]

Please add more details commit message that getExact in this place is only used for statistics collection, and since it has side effects it cannot be eliminated even if the collection is disabled.

Code-wise, it should be fine, but I'm not sure if it is OK to use this define outside of statistic collection infrastructure. I don't see precedents. If there is no guideline against that then I'm fine with it.

lib/Analysis/ScalarEvolution.cpp

6518 ↗

(On Diff #144290)

#endif  // LLVM_ENABLE_STATS

[javed.absar]

AFAIU the correctness check would now get gated on STATS colelction and that may not be right. I would recommend separating the two.

lib/Analysis/ScalarEvolution.cpp

6506 ↗

(On Diff #144290)

Please s/auto/"const SCEV *" to be consistent with other such instances in SCEV.

[skatkov]

assert now works.

[mkazantsev]

Fine for me (with the same doubt of direct usage of LLVM_ENABLE_STATS).

[javed.absar]

Please rephrase this part of commit message as it is hard to read/understand - "And even if statistics is disable and code
related to it will be eliminated the invocation to getExact itself will not be eliminated because it may have side-effects like creation of new SCEVs".

May be something like this - "Even if statistics is disabled, the code related to it will be eliminated the invocation to getExact itself will not be eliminated
because it may have side-effects like creation of new SCEVs". - if you agree with it

[skatkov]

Thank you for review guys. I'm ok with your version of commit message.

llvm/trunk/lib/Analysis/ScalarEvolution.cpp

ScalarEvolution::getBackedgeTakenInfo(const Loop *L) {
   if (!Pair.second)
     return Pair.first->second;
 
   // computeBackedgeTakenCount may allocate memory for its result. Inserting it
   // into the BackedgeTakenCounts map transfers ownership. Otherwise, the result
   // must be cleared in this scope.
   BackedgeTakenInfo Result = computeBackedgeTakenCount(L);
 
-  if (Result.getExact(L, this) != getCouldNotCompute()) {
-    assert(isLoopInvariant(Result.getExact(L, this), L) &&
+#if LLVM_ENABLE_STATS || !defined(NDEBUG)
+  const SCEV *BEExact = Result.getExact(L, this);
+  if (BEExact != getCouldNotCompute()) {
+    assert(isLoopInvariant(BEExact, L) &&
            isLoopInvariant(Result.getMax(this), L) &&
            "Computed backedge-taken count isn't loop invariant for loop!");
     ++NumTripCountsComputed;
   }
   else if (Result.getMax(this) == getCouldNotCompute() &&
            isa<PHINode>(L->getHeader()->begin())) {
     // Only count loops that have phi nodes as not being computable.
     ++NumTripCountsNotComputed;
   }
+#endif // LLVM_ENABLE_STATS || !defined(NDEBUG)
 
   // Now that we know more about the trip count for this loop, forget any
   // existing SCEV values for PHI nodes in this loop since they are only
   // conservative estimates made without the benefit of trip count
   // information. This is similar to the code in forgetLoop, except that
   // it handles SCEVUnknown PHI nodes specially.
   if (Result.hasAnyInfo()) {
     SmallVector<Instruction *, 16> Worklist;