[PATCH] D134614: [SCEV] Support clearing Block/LoopDispositions for a single value.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8388
+void ScalarEvolution::forgetBlockAndLoopDispositions(Value *V) {
+  if (V) {
+    if (const SCEV *S = getExistingSCEV(V)) {
----------------
mkazantsev wrote:
> ```
> if (!V)
>   return;
> ...
> ```
Simplified as suggested with a slight change to clear the full cache on this path, thanks!


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8396
+        const SCEV *Curr = Worklist.pop_back_val();
+        if (!LoopDispositions.erase(Curr))
+          continue;
----------------
mkazantsev wrote:
> I' not sure this is right. Imagine we have:
> ```
> a = ...
> x = a + 1;
> y = b + 1;
> ```
> Loop dispositions for `a` and `y` have been cached but not for `x`. If we stop at `x`, then `y` won't be invalidated.
> 
> Is this an impossible situation?
> Loop dispositions for a and y have been cached but not for x. If we stop at x, then y won't be invalidated.

I might be missing something, but a specific value should only be passed in for invalidation if only this value changed in the IR (e.g. because it as hoisted or sunk). So if `a`'s loop disposition becomes invalid, I think we would only need to invalidate `x`, because `y` doesn't use either `a` or `x`.

A slightly different scenario could be where `y` depends on `x`.

```
a = ...
x = a + 1;
y = x + 1;
```

Now if `x` would not be cached but `y` would, then we would not invalidate `y` at the moment.

I *think* this shouldn't happen, because when computing `y`'s loop disposition I think we require `x`'s disposition to already be known (ie. in the cache). From looking at the existing code, it doesn't look like current invalidation could introduce any such gaps.

Does that make sense? We could also go the more conservative route and remove the early continue, but I have a slight preference towards keeping it for now and revisit if any cases show up where the verifier runs into such gaps.





================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:8407
+      LoopDispositions.erase(S);
+      return;
+    }
----------------
mkazantsev wrote:
> not needed?
Gone in the re-structured version, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134614