Page MenuHomePhabricator

[NewPM] Revamp pass names
Needs ReviewPublic

Authored by aeubanks on Mar 1 2021, 12:51 PM.

Diff Detail

Event Timeline

aeubanks created this revision.Mar 1 2021, 12:51 PM
aeubanks requested review of this revision.Mar 1 2021, 12:51 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I'd like to get some feedback to see if this is reasonable before fixing up the tests and adding a description

bmahjour removed a subscriber: bmahjour.Mar 1 2021, 1:15 PM
ychen added a comment.Mar 1 2021, 11:21 PM

Thanks for doing this. I'm in favor of the patch to let pass explicitly specifying its own name. Previously we encountered many issues due to this restriction (one host-compiler-dependent name, one user-facing name, and translating between them). With that,

  • We have one name for both user-facing options and LLVM internally, the translation table is also dropped.
  • PassManager related tests don't need to be host-compiler-dependent anymore (PR48992 may be related to this and several regressions in the past).
  • Passes like NoOpModulePass are already doing this to work around test case issues. Now they are no longer special.

I don't see any cons with this approach other than it touches *a lot* of files. Guess that's the price we need to pay.

llvm/lib/Passes/PassBuilder.cpp
340

Delete.

I'd like to get some feedback to see if this is reasonable before fixing up the tests and adding a description

The downside of this approach is that we no longer see raw strings in PassRegistry.def which were aiding "grep"ing the names of passes. With this pass, developers will have to go to the pass, see its name() to find the name. Another downside, this patch touches a lot of files but since you have already went through that pain, it may no longer seen as pain point.

A slightly different approach:

  1. Have a static table of pass class name and its corresponding string representation. Something like,
static std::map<StringRef, StringRef> PassNames = {{"DDGAnalysis", "ddg"}, {"DependeceAnalysis", "da"}}
  1. Have a static function getName() in PassInfoMixin which uses __PRETTY_FUNCTION__ to get the name of the derived class to index into this table to retrieve its string representation. This is somewhat similar to current technique except that the name is now used as the index into the above table.

This way all derived mixins and their string representation would reside at one place and easy to look. Moreover, you could avoid touching all the files because PassInfoMixin is providing the definition and deriving the name using derived type name.

(FWIW, for 1, we can write TD files and a new a TableGen backend to emit the table.)

arsenm added inline comments.Mar 2 2021, 11:55 AM
llvm/include/llvm/Analysis/AliasAnalysis.h
1210

We already have multiple places where pass names are specified. We have getPassName, which many passes unnecessarily overload on top of the pass name set in the macros. Where does this version fit in this?

ychen added inline comments.Mar 2 2021, 11:59 AM
llvm/include/llvm/Analysis/AliasAnalysis.h
1210

getPassName is for legacy PM. This is the new PM counterpart.

I don't really like having C++ class names leak anywhere, it's at best implementation defined and as mentioned leads to incompatibilities between compilers.
If we create a table, we have to duplicate class names and make sure they're in sync which doesn't seem ideal. I'm trying to reduce the number of things to worry about with the new PM, namely the difference between the 2 types of pass names.
The legacy PM already didn't have C++ class names leaked and it seems like people were fine with not having a central list of class names for passes.

For passes added in *TargetMachine, currently the name is duplicated, but I do plan on a follow-up change to easily register passes which will solve that.

The purpose of C++ class names is to use it as key to the table. If you can
assign an unique key to each pass inherited from PassInfoMixin then that
will serve the purpose. But the idea of keeping a central map is useful for
programmers' productivity.

aeubanks updated this revision to Diff 327857.Mar 3 2021, 11:07 AM

add back in PassInfoMixin::name() as PassInfoMixin::className() and use that in --print-passes

I've made it so opt --print-passes will also display the class name alongside the pass name, does that work?

I've made it so opt --print-passes will also display the class name alongside the pass name, does that work?

Is that consistent with the old PM?

The legacy PM is already worse in that there's no way to find a pass declaration aside from grepping the name, and there's no centralized location of all the passes. (unless there's some mechanism I'm unaware of)

bjope added a subscriber: bjope.Mar 4 2021, 12:22 PM

I guess it wouldn't hurt if the pass names generally would match with the DEBUG_TYPE.

Using the same source of information is nice as it kind of ensures that the same name would be used when doing -debug-only=<name>, -start-before=<name>, -passes=<name>, -print-after=<name> etc. And I think that in the legacy PM this usually is handled by using the DEBUG_TYPE macro also when doing INITIALIZE_PASS_BEGIN etc (at least in most CodeGen passes).

A problematic thing with that here is that you can't simply move the DEBUG_TYPE definitions into the header files (well, it might be technically possible, but it might be problematic when including several header files that define the macros). One could ofcourse implement the name() methods in the the .cpp file, but that is also a bit messier implementation-wise.

uabelho added a subscriber: uabelho.Mar 5 2021, 2:56 AM

The legacy PM is already worse in that there's no way to find a pass declaration aside from grepping the name, and there's no centralized location of all the passes. (unless there's some mechanism I'm unaware of)

The options seems a good step to me then!

fhahn added a comment.Mar 6 2021, 10:54 AM

Thanks for tackling this!

I guess it wouldn't hurt if the pass names generally would match with the DEBUG_TYPE.
..
A problematic thing with that here is that you can't simply move the DEBUG_TYPE definitions into the header files (well, it might be technically possible, but it might be problematic when including several header files that define the macros). One could ofcourse implement the name() methods in the the .cpp file, but that is also a bit messier implementation-wise.

Tying it to DEBUG_TYPE would indeed be great. I guess the main downside with implementing the function in the .cpp is that it doubles the number of files touched, but I am not sure if it is really that messy in terms of the code.

Is it possible to add a test checking the new names? I don't think it's worth to check all of them, but I think it would be good to have a test that checks that we generate some of the compact names.

fhahn added a comment.Mar 6 2021, 10:55 AM

Is it possible to add a test checking the new names? I don't think it's worth to check all of them, but I think it would be good to have a test that checks that we generate some of the compact names.

Nevermind that bit, I missed your first comment.

2c from me:

  • there should be no single global map of pass names - how would that work with loadable/out-of-tree passes?
  • grepping for the pass name printed should not match some random source file, sending one on a further wild goose chase, but it should match the actual source (well, header isn't bad, either) where said pass is defined.

anything other seems hostile.