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

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.
This revision is now accepted and ready to land.

LGTM, but please wait a bit before committing (with Thanksgiving coming up, maybe till early next week?), in case someone more familiar with the SimpleLoopUnswitch internals wants to chime in.



================
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);
 
----------------
DaniilSuchkov wrote:
> 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.
Thanks for checking, I was mostly curious if there's something SimpleLoopUnswitch specific that makes us not requiring updating SCEV in fromLCSSA. I tried building SPEC2000, SPEC2006 & the MultiSource tests from test-suite with SimpleLoopUnswitch and scev-verify and everything passed. So in practice, we probably invalidate properly in most cases, but I agree it is better to be on the safe side here!


================
Comment at: llvm/test/Transforms/LCSSA/pr44058.ll:1
+; RUN: opt -passes="verify<scalar-evolution>,lcssa,verify<scalar-evolution>" -verify-scev-strict -S %s
+
----------------
DaniilSuchkov wrote:
> 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.
> 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.

Right, I am not sure what the best solution would be for that. Given that, having just the New PM version is probably fine, thanks!


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

https://reviews.llvm.org/D70593