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

[Maxim Kazantsev via llvm-commits]

Hi Sanjoy,

I've submitted the re-enabling for review as https://reviews.llvm.org/D36087 . I was able to make a unit test that exposes the problem which looks pretty similar to what you observed on your test. Please take a look once you have time.

Thanks,
Max

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

Ok, works for me!

-----Original Message-----
From: Sanjoy Das [mailto:sanjoy at playingwithpointers.com]
Sent: Friday, July 28, 2017 10:32 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

I have reverted this in r309357.

I can't share the test case I have, but you may be able to write a C++ test case for this.  And if you do have a fixed version, I can test it with the internal reproducer to make sure this bug is gone.

-- Sanjoy

On Thu, Jul 27, 2017 at 8:23 PM, Maxim Kazantsev <max.kazantsev at azul.com> wrote:
> 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