RFC: Add hyphens to pass name

IR & Optimizations
1

Currently, the naming convention of passes is very inconsistent.
Example:

 FUNCTION_PASS("lower-widenable-condition", LowerWidenableConditionPass()) // have hyphens
 FUNCTION_PASS("loweratomic", LowerAtomicPass()) // no hyphen

Add necessary hyphens could improve the readability and make command line options more consistent.

The concrete naming rule currently is not so clear.
Examples:

  • Use *2* or *-to-*?
  • Use domtree or dom-tree, instcombine or inst-combine?

Any suggestions would be appreciated.

2

CC @arsenm @boomanaiden154-1 @dblaikie @nikic

3

I’d prefer not to have any large scale renames. We should consider these on a case by case basis, depending on how inconsistent they are and how annoying the rename is going to be.

Renaming loweratomic to lower-atomic? Sure.

Renaming instcombine to inst-combine? No, thank you.

4 Likes
4

It’s not uncommon to need to add passes to clang invocations w/ -mllvm. If we do decide to rename things, it would be nice if we could let users know that a change is coming and give them time to adjust accordingly. IIRC our policy is to mark options as deprecated for a release, and then remove them unless the feature is broken.

Is there a way to mark the old pass name as deprecated so that users can migrate their builds away from using the old names? I’m not aware of a way to do that.

5

Unless there was something added recently, I’m unaware of an -mllvm flag to add passes via these textual pipelines.

We can print a deprecation message when we parse a specific name in PassBuilder.cpp.

But agreed with @nikic, I’d want to avoid any large scale changes, the benefits of more consistent naming seems low. (they are exposed to the C API, but otherwise these names are just for debugging/testing)

6

Oops, you’re right. Not sure why I thought that was the case. I had a vague memory that I had to do that, but looking at the example I had in mind its a frontend option.

In that case, I suppose my concern is far less relevant.

7

There are a couple passes that were ported to the NewPM pretty recently like loweratomic that can probably be renamed. Anything that made it into the version 17 release branch should probably stay as is in my opinion.

Going forward though, I think it would be good to specify a policy somewhere (coding standards, NewPM guide, both, or somewhere else) to have a specific policy that everyone agrees to that we can point at in the future to try and make sure that at least future additions are somewhat consistent.

8

Anyone have a strong reason not to allow multiple aliases? That seems like the easiest path towards a normalized naming scheme without breaking anyone’s workflow.

9

Doesn’t seem worth the complexity to me. I think there’s only a handful of high impact / commonly used pass names. The user facing break I’d be slightly concerned by would be for optimization remarks, where the filter functions leak the implementation detail of the pass names

10

Thanks for advice! I will pick pass names conservatively to reduce the impact and introduce a compatibility layer for new pass manager, but for legacy pass manager I couldn’t find a proper way to add alias.

11

Changing pass names (the names used in the cmd line API) could make it more painful when bisecting backwards to find out when bugs are introduced/resolved. Maybe acceptable if it’s done in one single commit, and then one would need to apply some script that updates the command line when bisecting past that commit.

I don’t think it is worth the complexity to change names. Specially for passes that has been around for years, and when lots of people are used to the current naming.

We did change several pass names when moving to NewPM. At that time it wasn’t such a big problem since the syntax for specifying the pipeline changed anyway.