Renaming passes

Hi,

As part of new pass manager work, I’ve been trying to get something like opt -foo working under the NPM, where foo is the name of a pass.

In the past there’s been no reason to keep the names of passes consistent between NPM and legacy PM. But now there is a reason to make them match, so that we don’t have to touch every single test that uses opt.

There are a couple of names that don’t match though, for example the “basic alias analysis” pass is named “basicaa” under the legacy PM
INITIALIZE_PASS_BEGIN(BasicAAWrapperPass, “basicaa”,
“Basic Alias Analysis (stateless AA impl)”, true, true)

but named “basic-aa” under the NPM
FUNCTION_ALIAS_ANALYSIS(“basic-aa”, BasicAA())

. Almost all the other AA passes have a dash in them so I think it makes sense to rename “basicaa” → “basic-aa”.

Is there accepted wisdom on renaming pass names? Is a pass name a stable interface? When is it ok to rename a pass? If there are 800 usages of a flag, should I rename them atomically?

I think the pass naming scheme needs a lot of work. The naming conventions seem random at times. For instance, I can never remember how to refer to PrologEpilogInserter. The DEBUG_TYPE name is “prologepilog”, the pass class name is “PEI”. I would expect this to be prolog-epilog-inserter to match the file and formal pass name, and consistently use dashes as word separators.

Unfortunately, I recently discovered some optimization pass remarks expose pass names to the user (since you use the name of the pass to filter relevant remarks). I’m not sure how stable this is supposed to be though.

-Matt

Can I suggest we allow aliases? We can except all of these names, pick a canonical name, migrate tests, and only remove the aliases once the new canonical names are widely known.

Hi,

As part of new pass manager work, I’ve been trying to get something like opt -foo working under the NPM, where foo is the name of a pass.

In the past there’s been no reason to keep the names of passes consistent between NPM and legacy PM. But now there is a reason to make them match, so that we don’t have to touch every single test that uses opt.

There are a couple of names that don’t match though, for example the “basic alias analysis” pass is named “basicaa” under the legacy PM
INITIALIZE_PASS_BEGIN(BasicAAWrapperPass, “basicaa”,
“Basic Alias Analysis (stateless AA impl)”, true, true)

but named “basic-aa” under the NPM
FUNCTION_ALIAS_ANALYSIS(“basic-aa”, BasicAA())

. Almost all the other AA passes have a dash in them so I think it makes sense to rename “basicaa” → “basic-aa”.

Is there accepted wisdom on renaming pass names? Is a pass name a stable interface? When is it ok to rename a pass? If there are 800 usages of a flag, should I rename them atomically?

I think the pass naming scheme needs a lot of work. The naming conventions seem random at times. For instance, I can never remember how to refer to PrologEpilogInserter. The DEBUG_TYPE name is “prologepilog”, the pass class name is “PEI”. I would expect this to be prolog-epilog-inserter to match the file and formal pass name, and consistently use dashes as word separators.

Can I suggest we allow aliases? We can except all of these names, pick a canonical name, migrate tests, and only remove the aliases once the new canonical names are widely known.

An alias sounds good.

In what contexts? I think aliases for the handful of potentially end user facing cases may be acceptable, but would worry about adding aliases everywhere. I think renaming things directly referring to a pass can be done pretty easily with a simple script?

-Matt

Hi,

As part of new pass manager work, I’ve been trying to get something like opt -foo working under the NPM, where foo is the name of a pass.

In the past there’s been no reason to keep the names of passes consistent between NPM and legacy PM. But now there is a reason to make them match, so that we don’t have to touch every single test that uses opt.

There are a couple of names that don’t match though, for example the “basic alias analysis” pass is named “basicaa” under the legacy PM
INITIALIZE_PASS_BEGIN(BasicAAWrapperPass, “basicaa”,
“Basic Alias Analysis (stateless AA impl)”, true, true)

but named “basic-aa” under the NPM
FUNCTION_ALIAS_ANALYSIS(“basic-aa”, BasicAA())

. Almost all the other AA passes have a dash in them so I think it makes sense to rename “basicaa” → “basic-aa”.

Is there accepted wisdom on renaming pass names? Is a pass name a stable interface? When is it ok to rename a pass? If there are 800 usages of a flag, should I rename them atomically?

I think the pass naming scheme needs a lot of work. The naming conventions seem random at times. For instance, I can never remember how to refer to PrologEpilogInserter. The DEBUG_TYPE name is “prologepilog”, the pass class name is “PEI”. I would expect this to be prolog-epilog-inserter to match the file and formal pass name, and consistently use dashes as word separators.

Can I suggest we allow aliases? We can except all of these names, pick a canonical name, migrate tests, and only remove the aliases once the new canonical names are widely known.

An alias sounds good.

In what contexts? I think aliases for the handful of potentially end user facing cases may be acceptable, but would worry about adding aliases everywhere. I think renaming things directly referring to a pass can be done pretty easily with a simple script?

My plan was to add an alias in PassNameParser, switch over all references to use the new one, then remove the alias. In order to avoid one change that touches 800 files. Also any rollbacks are easier.

Hi,

As part of new pass manager work, I’ve been trying to get something like opt -foo working under the NPM, where foo is the name of a pass.

In the past there’s been no reason to keep the names of passes consistent between NPM and legacy PM. But now there is a reason to make them match, so that we don’t have to touch every single test that uses opt.

What’s the goal here? Does it include removing the -passes option used by the NPM?

After talking with some NPM people, I believe the ultimate goal after NPM is enabled by default is to only support -passes=, and remove support for -foo-pass.
However, until NPM is enabled by default, we still want tests using opt to use the legacy PM by default.
We could attempt to make -passes= work with the legacy PM and have a legacy vs new PM flag, but given the design/syntax of -passes= I don’t think that’s feasible (see llvm/include/llvm/Passes/PassBuilder.h).
So for making sure everything works with NPM, I think we need to support -foo-pass in NPM to be able to run all opt tests against NPM. Then at some point after NPM is enabled by default we can attempt to migrate everything to -passes=.

After talking with some NPM people, I believe the ultimate goal after NPM is enabled by default is to only support `-passes=`, and remove support for `-foo-pass`.

Hm, is there any written rationale behind such a decision?
I would have thought that -passes= is the temporary solution, not the
other way around.

However, until NPM is enabled by default, we still want tests using opt to use the legacy PM by default.
We could attempt to make `-passes=` work with the legacy PM and have a legacy vs new PM flag, but given the design/syntax of `-passes=` I don't think that's feasible (see llvm/include/llvm/Passes/PassBuilder.h).
So for making sure everything works with NPM, I think we need to support `-foo-pass` in NPM to be able to run all opt tests against NPM. Then at some point after NPM is enabled by default we can attempt to migrate everything to `-passes=`.

Roman.

After talking with some NPM people, I believe the ultimate goal after NPM is enabled by default is to only support -passes=, and remove support for -foo-pass.
Hm, is there any written rationale behind such a decision?
I would have thought that -passes= is the temporary solution, not the
other way around.

This is really a separate issue that’s somewhat orthogonal to the original issue, but someone like asbirlea may be able to chime in more. Maybe a new RFC thread?

Agree with @Arthur Eubanks that we could leave the discussion (legacy PM versus new PM) on 1. Option compatibility 2. Pass name compatibility to the future. IMHO, the current issue about test correctness parity between legacy and new PM regarding pass name should be done with least disruptive way. Name alias seems to be proper to achieve that.

  • Yuanfang

Hi,

First of all, I agree that the discussion on whether to keep -passes=... or -foo-pass is a separate discussion and deserves its own RFC thread.

I don’t know of a written rationale for the decision to move forward only with passes=.... This was mostly discussed verbally, and no firm decision is yet in place. I believe the high-level motivation was the precision/clarity with which the passes=... format can describe a pass pipeline (one example from the recent compatibility patch is that NPM cannot have AA passes arbitrarily interleaved with module/function/loop, it’s using an AA pipeline instead).

I’d suggest having a broader discussion on pros/cons, either now or after the switch to the NPM, as the community prefers.

Thanks,
Alina

An alias facility in PassNameParser sounds good. Dropping the alias can
be a separate change so that rollback will be easier.

After talking with some NPM people, I believe the ultimate goal after NPM is enabled by default is to only support -passes=, and remove support for -foo-pass.
However, until NPM is enabled by default, we still want tests using opt to use the legacy PM by default.
We could attempt to make -passes= work with the legacy PM and have a legacy vs new PM flag, but given the design/syntax of -passes= I don’t think that’s feasible (see llvm/include/llvm/Passes/PassBuilder.h).
So for making sure everything works with NPM, I think we need to support -foo-pass in NPM to be able to run all opt tests against NPM. Then at some point after NPM is enabled by default we can attempt to migrate everything to -passes=.

I think many (but not all) test .ll files already have multiple RUN lines to run with both pass managers. If we do it in every test .ll file, would that work during the transition?

I don’t think that’s much less work than supporting -foo-pass, we still have to port all the passes to NPM. The infra for supporting -foo-pass is already done (I hope at least) and it reuses the same PassBuilder.parsePassPipeline() interface as -passes=. The only extra effort required is making sure pass names match up between the two pass managers.

And if we do go with adding -passes= to all tests and letting -foo-pass run under the legacy PM, then we’re sort of saying that everybody should migrate to -passes= since otherwise the test would be testing the legacy PM after the NPM switch, and that test’s value is diminished. As mentioned before this is a possibility, but a separate concern.