[PATCH] D60144: [SCEV] Add option to forget everything in SCEV.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

In D60144#1455099 <https://reviews.llvm.org/D60144#1455099>, @asbirlea wrote:

> Here are the times I got for compiling the aggregated bitcode files for clang and a few others. Before is ToT, After is with `-forget-scev-loop-unroll=true`
>  File    Before (s)   After (s)
>  clang-9.bc   7267.91   6639.14
>  llvm-as.bc   194.12   194.12
>  llvm-dis.bc   62.50   62.50
>  opt.bc   1855.85   1857.53
>
> The current test I have is not publishable, I'm still trying to get one.
>  In the mean time, the above results might motivate having this enabled by default.


These numbers look great!



================
Comment at: include/llvm/Transforms/Utils/UnrollLoop.h:66
 LoopUnrollResult UnrollLoop(Loop *L, unsigned Count, unsigned TripCount,
                             bool Force, bool AllowRuntime,
                             bool AllowExpensiveTripCount, bool PreserveCondBr,
----------------
Unrelated to this change, but the parameter lists are getting really long here.  What do you think about creating a struct holding all these params in a later change?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6792
 
+void ScalarEvolution::forgetEverything() {
+  BackedgeTakenCounts.clear();
----------------
What about `LoopPropertiesCache`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6793
+void ScalarEvolution::forgetEverything() {
+  BackedgeTakenCounts.clear();
+  PredicatedBackedgeTakenCounts.clear();
----------------
`forgetEverything` is not a good name for this function since e.g. we're not forgetting `UniqueSCEVs` (and we shouldn't).  Can we have a more specific name for this function and the boolean you're passing around?  Perhaps: `forgetAllLoops`?


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6796
+
+  ConstantEvolutionLoopExitValue.clear();
+  ValueExprMap.clear();
----------------
I also propose we should be explicit about what we're *not* clearing here (in the form of a comment) to not confuse future readers, and what the criteria should be to add a newly added cache to these of caches being cleared here.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:1135
+  /// If false, when SCEV is invalidated, only forget everything in the
+  /// forgetTopMostLoop, of the loop being processed. Otherwise,
+  /// forgetEverything and rebuild SCEV when needed next.
----------------
This didn't really parse, did you mean "only forget everything in the topmost loop"?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60144/new/

https://reviews.llvm.org/D60144