Page MenuHomePhabricator

[NFC] Add a unit test exposing lack of SCEV invalidation in LICM during code hoisting
ClosedPublic

Authored by DaniilSuchkov on Oct 24 2019, 2:16 AM.

Details

Summary

This unit test exposes a bug in LICM: when it hoists instructions it doesn't invalidate SCEV accordingly.
Similar test exposing lack of SCEV invalidation during code sinking will be submitted as a follow-up change.

Diff Detail

Event Timeline

DaniilSuchkov created this revision.Oct 24 2019, 2:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2019, 2:16 AM
reames requested changes to this revision.Oct 28 2019, 10:42 AM

I think you can write this test better as an lit test with strict validation. I assume SCEV has an expensive asserts mode - if not, we should add one - and thus this can be written as a lit test with an extra flag to opt.

This revision now requires changes to proceed.Oct 28 2019, 10:42 AM

I think you can write this test better as an lit test with strict validation. I assume SCEV has an expensive asserts mode - if not, we should add one - and thus this can be written as a lit test with an extra flag to opt.

There is such a mode in SCEV, but it is explicitly limited to backedge taken count verification (and even there some questionable cases are explicitly ignored). I'm not sure it will be easy to convince folks to extend it.
And you suggest to use extra SCEV validation to force SCEV to cache query results before LICM invocation, right? (to have the effect of line 70 from this test)

I think this is a great example of a unit test actually.
Getting to the scenario of forcing ScEv to cache the Load, so we can verify it is no longer properly cached after the Load is hoisted is harder to do in a lit test such that it's so easy to understand, like it is in this unit test.

I think @reames makes great point that we should have a more extensive validation for ScEv and be able to use that in lit tests. If you can take that on, I would be happy to look at the patch.
I don't think we should block the fix that depends on this patch on it though.

asbirlea accepted this revision.Oct 30 2019, 2:31 PM
This revision was not accepted when it landed; it landed in state Needs Revision.Oct 31 2019, 3:26 AM
This revision was automatically updated to reflect the committed changes.