This is an archive of the discontinued LLVM Phabricator instance.

[update_mir_test_checks.py] Use -NEXT FileCheck directories
ClosedPublic

Authored by arichardson on Sep 14 2021, 9:26 AM.

Details

Summary

Previously the script emitted output using plain CHECK directives. This
can result in a test passing even if there are some instructions between
CHECK directives that should have been removed. It also makes debugging
tests that have the output in a different order more difficult since
FileCheck can match with a later line and then complain about the "wrong"
directive not being found.

This will cause quite large diffs when updating existing tests, but I'm not sure we need an opt-in flag here.

Depends on D109765 (pre-commit tests)

Diff Detail

Event Timeline

arichardson created this revision.Sep 14 2021, 9:26 AM
arichardson edited the summary of this revision. (Show Details)Sep 14 2021, 9:31 AM
arichardson added reviewers: bogner, MaskRay, lebedev.ri.
arichardson published this revision for review.Sep 14 2021, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2021, 10:16 AM
MaskRay accepted this revision.Sep 14 2021, 10:31 AM

Looks great!

This revision is now accepted and ready to land.Sep 14 2021, 10:31 AM
This revision was landed with ongoing or failed builds.Sep 20 2021, 4:56 AM
This revision was automatically updated to reflect the committed changes.
aemerson reopened this revision.Sep 20 2021, 4:42 PM
aemerson added subscribers: arsenm, aemerson.

Wait...with this change we now have to regenerate every single MIR test before we make a change if it was using the old behavior. @arsenm what do you think?

I would prefer to have this behavior by opt-in.

This revision is now accepted and ready to land.Sep 20 2021, 4:42 PM

Wait...with this change we now have to regenerate every single MIR test before we make a change if it was using the old behavior. @arsenm what do you think?

I would prefer to have this behavior by opt-in.

Regenerating first will produce smaller diffs, but a sufficiently smart diff viewer (unfortunately not the GitHub one) will highlight only the actual changes (phabricator does an okay job at this). For larger changes I agree that it probably make sense to regenerate first.
I can add an opt-in flag, but IMO we should gradually transition all tests that are updated to the non-fragile -NEXT check lines instead of keeping the fragile CHECK lines that can allow tests to still pass even if certain instructions are no longer optimized out, etc.

arichardson closed this revision.Jan 17 2022, 5:54 AM