[PATCH] D70593: [LCSSA] Don't use VH callbacks to invalidate SCEV when creating LCSSA phis

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov marked 2 inline comments as done.
DaniilSuchkov added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:322
     // nodes for values used in the no-longer-nested loop.
-    formLCSSA(*OldContainingL, DT, &LI, nullptr);
+    formLCSSA(*OldContainingL, DT, &LI, SE);
 
----------------
fhahn wrote:
> DaniilSuchkov wrote:
> > This change is required to preserve current behavior in cases when it's valid: sometimes invalidation of SCEV through VH actually worked as intended.
> Do you have a test case that breaks when passing nullptr? After a quick scan of the code, it looks like SimpleLoopUnswitch forgets the whole loop when making changes (but I am not sure if that holds for all paths that use formLCSSA). But the SimpleLoopUnswitch tests all seem to pass when passing nullptr.
> Do you have a test case that breaks when passing nullptr?
No, don't have one. I have tried to write such a test but failed.

> After a quick scan of the code, it looks like SimpleLoopUnswitch forgets the whole loop when making changes (but I am not sure if that holds for all paths that use formLCSSA).
Yes, but for SCEV "forget the whole loop" doesn't include "forget all values defined within the loop".

But I personally don't want to find such a test by submitting this patch without any changes to SimpleLoopUnswitch and waiting for bugs to come.

By the way, if you wonder why only this pass was modified: it is because this pass is the only user of "formLCSSA" function outside LCSSAPass.


================
Comment at: llvm/test/Transforms/LCSSA/pr44058.ll:1
+; RUN: opt -passes="verify<scalar-evolution>,lcssa,verify<scalar-evolution>" -verify-scev-strict -S %s
+
----------------
fhahn wrote:
> if you're not checking the output, maybe pass `-disable-output`?
> 
> Also, can you also run using the legacy pass manager? It's still the default.....
> if you're not checking the output, maybe pass -disable-output?
Ok

> Also, can you also run using the legacy pass manager? It's still the default.....
To my knowledge there is no clear way to get effect of the first "verify<scalar-evolution>" invocation when using legacy PM. If I'm wrong and you know how to force SCEV cache population without tricks like "lets invoke a pass that (hopefully) won't transform anything but will (again, hopefully) perform SCEV queries we need", I will happily add a command for legacy PM too.


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

https://reviews.llvm.org/D70593