This is an archive of the discontinued LLVM Phabricator instance.

[test] Update some test cases to use -passes when specifying the pipeline
ClosedPublic

Authored by bjope on Aug 20 2021, 9:23 AM.

Details

Summary

This updates transform test cases for

ADCE
AddDiscriminators
AggressiveInstCombine
AlignmentFromAssumptions
ArgumentPromotion
BDCE
CalledValuePropagation
DCE
Reg2Mem
WholeProgramDevirt

to use the -passes syntax when specifying the pipeline.

Given that ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER is set to TRUE
the updated test cases already used the new pass manager, but they
were using the backwards compatibility hack to allow the legacy
syntax when specifying the passes to run. This patch can be seen as
a step toward deprecating that interface.

This patch also removes some redundant RUN lines (given that
ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER isn't overridden). Here I am
referring to test cases that had multiple RUN lines verifying both
"-passname" and "-passes=passname" syntax. I think that the
extra RUN line was added to verify that the pass worked with both
the legacy PM and new PM, but ever since we switched the default pass
manager to "new PM" both RUN lines have verified the new PM version
of the pass (more or less wasting time running the same test twice).

Diff Detail

Event Timeline

bjope created this revision.Aug 20 2021, 9:23 AM
bjope requested review of this revision.Aug 20 2021, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2021, 9:23 AM
nikic added a subscriber: nikic.Aug 20 2021, 9:52 AM

I'm not sure doing this is a good idea. The nice thing about the current way of testing is that it works both for the new pass manager and the legacy pass manager. That is, people can flip ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER to false, run the test suite, and get good test coverage for the legacy pass manager. Switching to NewPM-only syntax removes that ability.

For that reason I would expect that tests only get migrated to the new syntax once the legacy pass manager is officially unsupported (i.e. at least ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER and the corresponding clang option are removed), not the other way around. (I am of course in favor of doing that ASAP, now that the release branch has been cut.)

I'm not sure doing this is a good idea. The nice thing about the current way of testing is that it works both for the new pass manager and the legacy pass manager. That is, people can flip ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER to false, run the test suite, and get good test coverage for the legacy pass manager. Switching to NewPM-only syntax removes that ability.

How much work would it be to make --passes='foo,bar' be equivalent to -foo -bar for LPM? Whatever code does this could even complain that LPM is going away and hopefully nudge everyone into working on migrating to NPM.

bjope added a comment.Aug 24 2021, 4:00 AM

I'm not sure doing this is a good idea. The nice thing about the current way of testing is that it works both for the new pass manager and the legacy pass manager. That is, people can flip ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER to false, run the test suite, and get good test coverage for the legacy pass manager. Switching to NewPM-only syntax removes that ability.

How much work would it be to make --passes='foo,bar' be equivalent to -foo -bar for LPM? Whatever code does this could even complain that LPM is going away and hopefully nudge everyone into working on migrating to NPM.

Since not all pass names have a one-to-one mapping etc I doubt that it is worth the effort to introduce something like that.

I think that what we want to do is to start reducing amount of compatibility hacks that we need to maintain for the pass manager switch (such as dropping support for using the legacy syntax together with the new-PM) rather than adding yet another temporary hack.

As @nikic pointed out, we probably want to remove the ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER option first. Kind of forcing the default in opt to be the new PM. After that one would need to explicitly use -enable-new-pm=0 to get the legacy PM instead. Given that first step we can start to do cleanup such as this patch, but also probably remove the possibility to use -enable-new-pm=1 and the old legacy syntax for specifying which opt passes to run unless using -enable-new-pm=0 .

I don't think we're missing much coverage by removing some tests that run against the legacy PM.

But point taken on the legacy PM deprecation, sent out an RFC.

Let me know when we can drop legacy PM testing for these passes.

We've agreed in llvm-dev that we can start removing legacy PM tests

Matt added a subscriber: Matt.Sep 10 2021, 10:12 AM
aeubanks accepted this revision.Sep 29 2021, 10:10 AM
This revision is now accepted and ready to land.Sep 29 2021, 10:10 AM