Page MenuHomePhabricator

[LCSSA] Don't use VH callbacks to invalidate SCEV when creating LCSSA phis
ClosedPublic

Authored by DaniilSuchkov on Nov 22 2019, 3:52 AM.

Details

Summary

In general ValueHandleBase::ValueIsRAUWd shouldn't be called when not all uses of the value were actually replaced, though, currently formLCSSAForInstructions calls it when it inserts LCSSA-phis.

Calls of ValueHandleBase::ValueIsRAUWd were added to LCSSA specifically to update/invalidate SCEV. In the best case these calls duplicate some of the work already done by SE->forgetValue, though in case when SCEV of the value is SCEVUnknown, SCEV replaces the underlying value of SCEVUnknown with the new value (i.e. acts like LCSSA-phi actually fully replaces the value it is created for), which leads to SCEV being corrupted because LCSSA-phi rarely dominates all uses of its inputs.

This fixes bug https://bugs.llvm.org/show_bug.cgi?id=44058.

Diff Detail

Event Timeline

DaniilSuchkov created this revision.Nov 22 2019, 3:52 AM
DaniilSuchkov retitled this revision from [LCSSA] Don't use VH callbacks to invalidate SCEV to [LCSSA] Don't use VH callbacks to invalidate SCEV when creating LCSSA phis.Nov 22 2019, 3:53 AM
DaniilSuchkov marked an inline comment as done.Nov 22 2019, 3:57 AM
DaniilSuchkov added inline comments.
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
322

This change is required to preserve current behavior in cases when it's valid: sometimes invalidation of SCEV through VH actually worked as intended.

Are the changes to loop unswitch supposed to be in this patch?

Are the changes to loop unswitch supposed to be in this patch?

Yes. I knew it won't be obvious, so I left an inline comment:

This change is required to preserve current behavior in cases when it's valid: sometimes invalidation of SCEV through VH actually worked as intended.

fhahn added inline comments.Nov 26 2019, 12:28 PM
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
322

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.

llvm/test/Transforms/LCSSA/pr44058.ll
2

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.....

DaniilSuchkov marked 2 inline comments as done.Nov 27 2019, 2:59 AM
DaniilSuchkov added inline comments.
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
322

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.

llvm/test/Transforms/LCSSA/pr44058.ll
2

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.

fhahn accepted this revision.Nov 27 2019, 5:00 AM

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.

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
322

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!

llvm/test/Transforms/LCSSA/pr44058.ll
2

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!

This revision is now accepted and ready to land.Nov 27 2019, 5:00 AM
This revision was automatically updated to reflect the committed changes.