[llvm] r309080 - [SCEV] Cache results of computeExitLimit

[Maxim Kazantsev via llvm-commits]

Thanks for pointing out this problem Sanjoy!

Do you have an IR test case for the problem? I'll make the re-enabling of this patch my priority, but it would be extremely nice to have a test that shows that the probmen is gone. :)

-- Max

-----Original Message-----
From: Sanjoy Das [mailto:sanjoy at playingwithpointers.com] 
Sent: Friday, July 28, 2017 10:18 AM
To: Maxim Kazantsev <max.kazantsev at azul.com>
Cc: llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [llvm] r309080 - [SCEV] Cache results of computeExitLimit

Hi Max,

I think this patch is buggy and I'm about to revert it (it breaks some Google-internal workload).  As far as I can tell, you need to clear out the cache in forgetMemoizedResults like this:

diff --git a/include/llvm/Analysis/ScalarEvolution.h
b/include/llvm/Analysis/ScalarEvolution.h
index 226e9881f10..02d9b0e5496 100644
--- a/include/llvm/Analysis/ScalarEvolution.h
+++ b/include/llvm/Analysis/ScalarEvolution.h
@@ -610,6 +610,8 @@ private:
              !isa<SCEVCouldNotCompute>(MaxNotTaken);
     }

+    bool hasOperand(const SCEV *S) const;
+
     /// Test whether this ExitLimit contains all information.
     bool hasFullInfo() const {
       return !isa<SCEVCouldNotCompute>(ExactNotTaken);
diff --git a/lib/Analysis/ScalarEvolution.cpp b/lib/Analysis/ScalarEvolution.cpp
index b7421aa3c0d..eb0d249dd1c 100644
--- a/lib/Analysis/ScalarEvolution.cpp
+++ b/lib/Analysis/ScalarEvolution.cpp
@@ -10805,6 +10805,14 @@ bool ScalarEvolution::hasOperand(const SCEV *S, const SCEV *Op) const {
   return SCEVExprContains(S, [&](const SCEV *Expr) { return Expr == Op; });  }

+bool ScalarEvolution::ExitLimit::hasOperand(const SCEV *S) const {
+  auto IsS = [&](const SCEV *X) { return S == X; };
+  return (!isa<SCEVCouldNotCompute>(ExactNotTaken) &&
+          SCEVExprContains(ExactNotTaken, IsS)) ||
+         (!isa<SCEVCouldNotCompute>(MaxNotTaken) &&
+          SCEVExprContains(MaxNotTaken, IsS)); }
+
 void ScalarEvolution::forgetMemoizedResults(const SCEV *S) {
   ValuesAtScopes.erase(S);
   LoopDispositions.erase(S);
@@ -10838,6 +10846,14 @@ void
ScalarEvolution::forgetMemoizedResults(const SCEV *S) {

   RemoveSCEVFromBackedgeMap(BackedgeTakenCounts);
   RemoveSCEVFromBackedgeMap(PredicatedBackedgeTakenCounts);
+
+  SmallVector<ExitLimitQuery, 8> ToDelete;  for (auto KV : ExitLimits)
+    if (KV.second.hasOperand(S))
+      ToDelete.push_back(KV.first);
+
+  for (auto K : ToDelete)
+    ExitLimits.erase(K);
 }

 void ScalarEvolution::verify() const {

(I'm not a big fan of how slow this way of clearing out the cache is, btw; so I'd be super happy if you can come up with a better mechanism to do the same thing :)).


-- Sanjoy