[MLIR] Split autogenerated pass declarations & C++ controllable pass options

[mscuttari]

Summary

The pass tablegen backend has been reworked to remove the monolithic nature of the autogenerated declarations.
The pass public header can be generated with the -gen-pass-decls option. It contains options structs and registrations: the inclusion of options structs can be controlled individually for each pass by defining the GEN_PASS_DECL_PASSNAME macro; the declaration of the registrations have been kept together and can still be included by defining the GEN_PASS_REGISTRATION macro.
The private code used for the pass implementation (i.e. the pass base class and the constructors definitions, if missing from tablegen) can be generated with the -gen-pass-defs option. Similarly to the declarations file, the definitions of each pass can be enabled by defining the GEN_PASS_DEF_PASNAME variable.
While doing so, the pass base class has been enriched to also accept a the aformentioned struct of options and copy them to the actual pass options, thus allowing each pass to also be configurable within C++ and not only through command line.

[Mogball]

mlir/tools/mlir-tblgen/PassGen.cpp

314

Accessing the options class will be awkward because the base class takes a template parameter.

[mehdi_amini]

Thanks, tou need to add a few tests now :)

mlir/tools/mlir-tblgen/PassGen.cpp

87

When removing this I can't tell what you're making public right now.

[mscuttari]

mlir/tools/mlir-tblgen/PassGen.cpp

87

That has been moved in the "code generating" emitPassDecl method. The code that was generated after that protected keyword is still protected. The Options struct and the constructor, however, needs to be public and thus the "removal".

314

What do you mean with awkward?

[mscuttari]

mlir/tools/mlir-tblgen/PassGen.cpp

314

Oh sorry, realized later: you would need to write something like MyPassBase<MyPassImpl>::Options. Having not written a test didn't show me this problem. I will think about it (any suggestion is welcome)

[mscuttari]

I’ve made few changes:

1. Moved the options struct outside the pass class. I would have gone for the “PassNameOptions” name, but this was creating some issues with other existing passes such as LinalgTiling for which a LinalgTilingOptions already exists. I don’t have enough knowledge and time to adapt all the passes, thus I have opted to go for the “PassNameBaseOptions” name. When all the passes will switch to the tablegen-defined options, then the name can be changed.
2. Restored the protected keyword. With the new design it can stay in the template string.
3. Added a test to check the options forwarding.

[mscuttari]

The generation of the options structure has been separated from the generation of the pass classes. An additional compile-tme definition GEN_PASS_OPTIONS has been introduced to
include such structs. The rationale behind this choice is the need of including the options structures in the top-level Conversion/Passes.h, thus allowing them to be used
from outside the pass implementations (i.e. by users wanting to customize the behaviour of the pass).
In case no option exists for a pass, the struct is not generated (it would be empty).
The existing conversion passes have also been adapted to work with this modified infrastructure.

[mehdi_amini]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

How does one get the definition for this?

mlir/include/mlir/Conversion/GPUToNVVM/GPUToNVVMPass.h

41–45 ↗

(On Diff #452662)

I think the comment is still relevant though

mlir/include/mlir/Conversion/GPUToSPIRV/GPUToSPIRVPass.h

28–29 ↗

(On Diff #452662)

The last two lines are irrelevant to this constructor.

mlir/lib/Conversion/ControlFlowToSPIRV/ControlFlowToSPIRVPass.cpp

30 ↗

(On Diff #452662)

Isn't using ConvertControlFlowToSPIRVBase::ConvertControlFlowToSPIRVBase; enough here to inherit the constructors?

[mscuttari]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

The definition is inside the autogenerated Passes.h.inc, and is wrapped by an #ifdef PASS_GEN_OPTIONS. I don't know if this is the best design, but that's the only coming to my mind. I would apreciate some opinions about this.

mlir/include/mlir/Conversion/GPUToNVVM/GPUToNVVMPass.h

41–45 ↗

(On Diff #452662)

The Passes.td file already has a description for the index option. Imho keeping the comment would be incomplete wrt the other options.

mlir/include/mlir/Conversion/GPUToSPIRV/GPUToSPIRVPass.h

28–29 ↗

(On Diff #452662)

NIce catch, thanks. I will remove them.

mlir/lib/Conversion/ControlFlowToSPIRV/ControlFlowToSPIRVPass.cpp

30 ↗

(On Diff #452662)

Also another nice suggestion :) I will fix this together with all the other constructors.

[mehdi_amini]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

The definition is inside the autogenerated Passes.h.inc, and is wrapped by an #ifdef PASS_GEN_OPTIONS

Right, but my question is more immediate: what header should someone interested in creating the ConvertControlFlowToLLVMPass with some options use to be able to set these options?

[mscuttari]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

mlir/Conversion/Passes.h. That header has the GEN_PASS_OPTIONS define and the Passes.h.inc inclusion, and thus the definitions of the option structures. The weird thing to my eyes is that I have to include the top level header (and not, for example, the ConvertControlFLowToLLVM.h one), and this is where I would like opinions

[mscuttari]

I also see that these changes break flang. Maybe we should temporarily add an option inside the pass tablegen, in order to enable the structs generation, and just tell people to start adapting to this?

[mehdi_amini]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

Right, I think this is a more profound issue, I think we should change the way we handle passes to be able to have one header per pass, and kill entirely the mlir/Conversion/Passes.h style (other than for the global registration entry point).

That is I could see:

#define GEN_PASS_DECL_ConvertControlFlowToLLVMPass
#include "mlir/Conversion/Passes.h.inc"

And:

#define GEN_PASS_DEF_ConvertControlFlowToLLVMPass
#include "mlir/Conversion/Passes.h.inc"

Which would generate not only the pass options but also the declaration and definition for the createConvertControlFlowToLLVMPass entirely.

[mehdi_amini]

In D131839#3723260, @mscuttari wrote:

I also see that these changes break flang. Maybe we should temporarily add an option inside the pass tablegen, in order to enable the structs generation, and just tell people to start adapting to this?

In general I rather have the "right" default and provide an opt-out flag: that let people adapt to it but when they opt out they at least do so conscientiously and can add a "todo" and/or a tracking bug for upgrading.

[mscuttari]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

You may not believe it, but I thought about generating that specialized define for just options and I stopped because I considered it a bit ugly for users. Glad we both arrived to the same solution :). I will give a try asap and report back.

[mscuttari]

In D131839#3723305, @mehdi_amini wrote:

In D131839#3723260, @mscuttari wrote:

I also see that these changes break flang. Maybe we should temporarily add an option inside the pass tablegen, in order to enable the structs generation, and just tell people to start adapting to this?

In general I rather have the "right" default and provide an opt-out flag: that let people adapt to it but when they opt out they at least do so conscientiously and can add a "todo" and/or a tracking bug for upgrading.

I agree about that, but in this case I would have to modify the whole flang project to usethat opt-out flag. At that point, I would just fix it to use the new infrastructure. Atm the build bots are screaming about flang not being coherent with the new changes (the new changes we just discussed will come, but I don't think they will improve the situation).

[mravishankar]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

Wouldnt it be a less intrusive change if the generation of this options struct is controllable from the tablegen definition of the pass, say, a boolean field that when set generates the options struct. Then you could also generate the constructor for the pass that takes the generated struct as argument and initializes the pass options. Its upto user to then either have a new create method or not....

[mehdi_amini]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

Right, we could trigger this on whenever the let constructor = isn't present?

Could also add a flag let generatePassOptions = 1;, but I'd want this to be the default and it is much harder to update every single pass to opt-out rather than the TableGen invocation (for flang isn't it a simple grep to find the instances followed by a sed to add the tablegen flag to opt out?)

[Mogball]

How are you supposed to access the pass options struct outside of the libraries? The pass class definitions aren't exposed publicly and you've added the struct definitions to the PassDetail.h files, which are not visible outside the libraries implementation code.

mlir/lib/Dialect/Transform/Transforms/CheckUses.cpp

364–368 ↗

(On Diff #452662)

mlir/tools/mlir-tblgen/PassGen.cpp

35

I would class this {0}PassOptions

[Mogball]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

Which would generate not only the pass options but also the declaration and definition for the createConvertControlFlowToLLVMPass entirely.

+1

[mscuttari]

This comment was removed by mscuttari.

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

To be honest I don't know about flang. I've looked into it only in my early days with mlir to understand some things.

23 ↗

(On Diff #452662)

So t

[mscuttari]

The options structs are declared inside the mlir/Conversion/Passes.h header. PassDetails.h includes only the pass base classes.
So, just to confirm, do we want a GEN_PASS_PassName define that enables both the options struct and the pass base class? Shouldn't the base class be available only in the PassDetails.h header? With a single define we either miss the public visibility of options (which makes all this useless) or have to accept the base classes having public visibility.

P.S: sorry for messing up with inline comments. Still quite new with the platform.

[Mogball]

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #452662)

You will need to fix the flang build

[mehdi_amini]

So, just to confirm, do we want a GEN_PASS_PassName define that enables both the options struct and the pass base class? Shouldn't the base class be available only in the PassDetails.h header?

There are two GEN_PASS_PassName I mentioned: one for the public header and the other for the private implementation. The Base class goes in the private part.

[mscuttari]

In D131839#3724243, @mehdi_amini wrote:

So, just to confirm, do we want a GEN_PASS_PassName define that enables both the options struct and the pass base class? Shouldn't the base class be available only in the PassDetails.h header?

There are two GEN_PASS_PassName I mentioned: one for the public header and the other for the private implementation. The Base class goes in the private part.

Oh yes sorry, I wrongly read them as if they had the same name. I proceed in implementing the required changes.

[mscuttari]

Introduced both the PASS_GEN_DECL_PassName and PASS_GEN_DEF_PassName. The former allows to import the options struct and the constructors (createPassNamePass() and createPassNamePass(const PassNamePassOptions &options)); these two methods are automatically declared in case of a missing constructor inside the pass TableGen. The latter allows to import the definition of the pass base class.
The declarations can be emitted through the option -gen-pass-decls, like it was before, while the definitions can be generated with the new option -gen-pass-defs.

[mscuttari]

I've introduced what we have been discussing yesterday. The flang build still needs to be fixed, but I would like to first reach consensus on the design.
An update wrt one of my previous answers: the definition of struct containing a pass options can now be obtained by including the header of the pass (e.g ConvertMemRefToLLVMPassOptions can be obtained by including mlir/Conversion/MemRefToLLVM/MemRefToLLVM.h)

[mehdi_amini]

Wow! You didn't have to update all the passes to adopt this new feature from the get go.

Actually I tend to prefer to land the TableGen feature with the test dialect, and adopt it widely in one or multiple subsequent commits. This also help to show when something is backward compatible: can you clarify if this change is gonna be a breakage for all users (that is you had to update the entire codebase?).

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #453114)

Is this forward declaration still needed here?

mlir/include/mlir/Conversion/Passes.td

104 ↗

(On Diff #453114)

This may show a limitation of the current approach: we can't generate the constructor in a namespace right?

[mscuttari]

In D131839#3727478, @mehdi_amini wrote:

Wow! You didn't have to update all the passes to adopt this new feature from the get go.

Actually I tend to prefer to land the TableGen feature with the test dialect, and adopt it widely in one or multiple subsequent commits. This also help to show when something is backward compatible: can you clarify if this change is gonna be a breakage for all users (that is you had to update the entire codebase?).

Well, I had to update the entire codebase due to the different autogenerated file and definitions (i.e. GEN_PASS_DECL_PassName, GEN_PASS_DEF_PassName), but it is also true that I can potentially still emit also the old stuff (under GEN_PASS_CLASSES) and thus avoid the breakage for other users. Still, at some point (that is when we decide to switch to the new design) they will have to adapt their code, and this is the reason why I preferred to update the whole MLIR project from the beginning. But I understand your point, and maybe a transitioning tablegen-ed files and individual patches for all the passes may be better in terms of readability (and potential revertibility). If the temporary coexistence of both old and new stuff is ok, I will switch to that.

mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h

23 ↗

(On Diff #453114)

No, I just missed it. I will remove it in the next diff.

mlir/include/mlir/Conversion/Passes.td

104 ↗

(On Diff #453114)

I'm not sure I get your question, atm the "constructor" (that is, the function returning the pass instance) is not generated at all and the constructor entry in tablegen is used only to make the call within the registration method. The user manually declares the createConvertArithmeticToLLVMPass() method inside the mlir::arith namespace.
With the new approach, if you rely on the constructors to be generated by tablegen, they will be placed within the namespace sorrounding the .h.inc include. If you want a different namespace, then you have to manually generate them and specify the constructor through tablegen (like before).
Side note: while patching the codebase I noticed that some constructors are within the mlir namespace, while others aren't (e.g the above one). As a common rule, I opted to keep the conversion declarations within the mlir namespace, while the dialect specific transforms are left nested in the dialect specific namespace.
Also, one more thing to notice is that with the proposed patch the autogenerated constructors have the std::unique_ptr<mlir::Pass> return type, while a lot of passes use std::unique_ptr<mlir::OperationPass<>> or std::unique_ptr<mlir::OperationPass<mlir::ModuleOp>>, and thus they still need to be manually declared.

[mehdi_amini]

If the temporary coexistence of both old and new stuff is ok, I will switch to that.

In this case that seems desirable to me, if it is doable without overly convoluted heroics.

[mscuttari]

The old pass base class declarations are emitted in order to preserve backward compatibility.
The test reflects the usage of the patched infrastructure.

[mscuttari]

Formatting fix. Somehow, this time git-clang-format didn't warn me (!?).

[Mogball]

The direction of this patch LGTM. Splitting up the pass declarations to not be monolithic has been something people wanted

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

94

This function is called once. Can you just inline it?

97

Drop braces

98

+1

mlir/tools/mlir-tblgen/PassGen.cpp

392

I'm hesitant to accept supporting "backwards compatibility" without a timeline to actually migrate all the upstream passes over (which you have already done, actually, so hopefully this timeline is small!) and deleting the old one.

[Mogball]

mlir/include/mlir/Pass/PassBase.td

79

Can you please use an empty string to indicate "not present"? It's not possible to pass ? as a template argument in tablegen, so the former is much more convenient for writing something like

class FooPass<string ctor = ""> : Pass {
  let constructor = ctor;
}

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

95

Please spell out this auto

102

A string stream here isn't necessary. You can just call formatv(...).str()

mlir/tools/mlir-tblgen/PassGen.cpp

104

I would prefer to uppercase passName here, to get GEN_PASS_DECL_MYPASSNAME. @mehdi_amini thoughts?

124

Ditto. Can you just inline this?

[Mogball]

This patch also has two things munged together: the pass options and reworking the way passes are generated. I personally don't mind that they're munged together, but can you edit the patch title and description to document all of these changes?

[rriddle]

I haven't had time to review, but will look later today.

[mscuttari]

Address comments

mlir/tools/mlir-tblgen/PassGen.cpp

104

Sounds good to me, but I will wait also for other opinions before changing

392

As you can see from my previous diff, updating the whole codebase in just one patch is quite huge (also, there are sometimes some "issues" with namespaces which may be worth discussing individually with whoever takes care of that part). I will split that into multiple commits (one for each pass), and being the work already done it shouldn't take long, as you correctly said.

[lattner]

Nice, I'm excited to see a less monolithic approach to GEN_PASS_CLASSES, this will help for passes.td files with multiple passes that have differing dialect requirements etc. Right now you have to forward declare a lot just because they are used by other passes. very cool!

[mehdi_amini]

Is there any doc to update?

mlir/tools/mlir-tblgen/PassGen.cpp

104

I"m fine with UPPER :)

[mehdi_amini]

LG, but please wait for @Mogball to approve!

[jpienaar]

Thanks!

mlir/tools/mlir-tblgen/PassGen.cpp

104

SGTM

[rriddle]

This is missing documentation updates to PassManagement.md.

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

101–103

mlir/tools/mlir-tblgen/PassGen.cpp

33

This should be a static method.

[Mogball]

lgtm. Please address river's comments

mlir/include/mlir/Pass/PassBase.td

79–83

mlir/tools/mlir-tblgen/PassGen.cpp

392

with whoever takes care of that part

I would just throw up one huge patch that migrates all the passes at once, to be honest, since the work is already done. The individual changes are small enough that it's not too burdensome. Can you upload the patch and mark it as Depends on D131839?

mlir/unittests/TableGen/PassGenTest.cpp

42

It's unfortunate that these can't be autogenerated as well due to the CRTP base class.

[mscuttari]

In D131839#3730050, @rriddle wrote:

This is missing documentation updates to PassManagement.md.

I proceed to update it.

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

101–103

Should I remove that whiteline? Isn't it a big ugly without?

mlir/tools/mlir-tblgen/PassGen.cpp

33

Nice catch thanks, missed it through the various changes.

[rriddle]

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

101–103

I wouldn't say ugly (at least not to me). Stylistically I effectively never see cases with a newline in between (or at least, it's very uncommon)

mlir/unittests/TableGen/PassGenTest.cpp

42

Technically we could if we enforced that the pass name was the same as the ODS def name (e.g. with some static assert or something).

[Mogball]

mlir/unittests/TableGen/PassGenTest.cpp

42

Not a bad idea. Not having to manually define these default constructors that are mostly all identical would be nice!

[mscuttari]

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

101–103

No problem, removing :)

mlir/unittests/TableGen/PassGenTest.cpp

42

I think I'm missing something. How would the definition look like? Tablegen doesn't know the derived class to be instantiated.

// .h.inc
std::unique_ptr<mlir::Pass> createTestPass();

// cpp.inc
class Test : public TestBase {
...
}

std::unique_ptr<mlir::Pass> createTestPass() {
  return std::make_unique<?>(); // <-- TestBase ?
}

[mscuttari]

mlir/unittests/TableGen/PassGenTest.cpp

42

Oh well, I think I got what you mean (it's summer, my brain slows down :D). You just want a static assert into the base constructor checking for the derived class name to be equal to the one defined in tablegen. Atm I don't exactly know how to perform that check though.

[mehdi_amini]

mlir/unittests/TableGen/PassGenTest.cpp

42

Remove "Base" suffix and done?

I actually missed that you were not generating these yet, I thought the _DEF macro would :)

[mscuttari]

mlir/tools/mlir-tblgen/PassGen.cpp

392

I have to update it with the latest changes first. I will try to do it tomorrow but I'm not sure I will be able to gather enough time to complete it. I will be on vacation for the next few days and in case I will upload it again on 24th or 25th.
Still I don't know how much would be easy to review the patch. I feel like reaching consensous on such a large amount of modified files would require involving a lot of people into a single review while at the same time increasing the chance of missing wrong modifications. Also, I still need to look into flang and the "huge" patch would be incomplete anyway.
@mehdi_amini any opinion about this?

mlir/unittests/TableGen/PassGenTest.cpp

42

I'm thinking about it and it may effectively be that simple.
With a tablegen file looking like:

def TestPass : PassBase ... {
...
}

We would get the following:

// .h.inc
std::unique_ptr<mlir::Pass> createTestPass();
std::unique_ptr<mlir::Pass> createTestPass(const TestPassOptions &options)

// .cpp.inc
class TestPass : public TestPassBase {
...
}

std::unique_ptr<mlir::Pass> createTestPass() {
  return std::make_unique<TestPass>();
}

std::unique_ptr<mlir::Pass> createTestPass(const TestPassOptions &options) {
  return std::make_unique<TestPass>(options);
}

Also, one unrelated consideration: the patched tablegen would emit a class named TestPassPassOption, should we append just "Options" instead of "PassOptions"? I see some passes already do define such a class though, and thus would clash. Should we just go on and then fix the clashing passes when updating them to use the new infra?

[rriddle]

mlir/tools/mlir-tblgen/PassGen.cpp

392

You don't need a review from *every* person for that stuff, just add me, @mehdi_amini, @jpienaar, @Mogball etc.

mlir/unittests/TableGen/PassGenTest.cpp

42

Also, one unrelated consideration: the patched tablegen would emit a class named TestPassPassOption, should we append just "Options" instead of "PassOptions"? I see some passes already do define such a class though, and thus would clash. Should we just go on and then fix the clashing passes when updating them to use the new infra?

We should decide on the "right" way to define these and just handle that, and then fix up the current places defining it incorrectly. We intended to do this while ago, but never had time.

[mehdi_amini]

mlir/tools/mlir-tblgen/PassGen.cpp

392

Sharding the updates to various community members owning areas is fine to me.

We've been doing this for other things, like the getters/setters: the project at some point becomes too big to require someone adding a new feature to migrate everyone.
(Are we using declarative assembly everywhere? Including in flang?)

mlir/unittests/TableGen/PassGenTest.cpp

42

I think it should be:

// .cpp
class TestPass : public TestPassBase {
...
}

// .cpp.inc
std::unique_ptr<mlir::Pass> createTestPass() {
  return std::make_unique<TestPass>();
}
...

That is: the pass is user-written and not generated, by design.

[jpienaar]

mlir/unittests/TableGen/PassGenTest.cpp

42

You'll see remnants of a patch where I started fixing the PassPass generation in name but never finished and I never got back to ... ( there I had gone for all Pass's should have Pass suffix as that match how we do Ops, Types and Attr.). So +1 to River's comment here.

[mscuttari]

mlir/unittests/TableGen/PassGenTest.cpp

42

We should decide on the "right" way to define these and just handle that, and then fix up the current places defining it incorrectly. We intended to do this while ago, but never had time.

No problem, we all have our stuff to do. My two cents: partially answering to myself, we should just keep appending "Pass" to the pass name and "PassOptions" for the options struct. This way, passes like ConvertXToY would become ConvertXToYPass and generate the createConvertXToYPass constructor (and NOT createConvertXToY, which is a bit weird), while passes like Bufferize would generate BufferizePass and createBufferizePass (and not createBufferize).
The above example is just unfortunate because of the already existing 'Pass' inside the name.

[mscuttari]

mlir/unittests/TableGen/PassGenTest.cpp

42

You'll see remnants of a patch where I started fixing the PassPass generation in name but never finished and I never got back to ... ( there I had gone for all Pass's should have Pass suffix as that match how we do Ops, Types and Attr.). So +1 to River's comment here.

So if I get it right you're suggesting the opposite path wrt to my last comment, assuming that the user manually writes the Pass suffix in the tablegen declaration? (asking just clarity, I don't have a strong opinion about this).

I think it should be:

Right, it matches with my example.

[mehdi_amini]

mlir/unittests/TableGen/PassGenTest.cpp

42

Right, it matches with my example.

To be clear: the comments // .cpp and // .cpp.inc differ from your previous example, unless I am missing what you're referring to.

[mscuttari]

mlir/unittests/TableGen/PassGenTest.cpp

42

You're right, and it's a good thing you made me notice that because I'm seeing one more problem in the constructor definition. GEN_PASS_DEF_PASSNAME wraps the pass base class, and now it would also wrap the create method. However, the actual pass class has to be implemented by the user, and thus first needs to include the .cpp.inc. But the constructor then would come before the class definition. It would be equivalent to manually writing this into the .cpp, which would not compile:

// Automatically generated
class TestBase {
    ...
};

// Automatically generated
std::unique_ptr<Pass> createTestPass() {
    return std::make_unique<TestPass>();
}

// Manually defined by the user
class TestPass : public TestBase {
    
};

On the other hand, moving the #include .cpp.inc after the TestPass class definition would lead to TestBase not being defined.

[mehdi_amini]

mlir/unittests/TableGen/PassGenTest.cpp

42

Ah! Annoying... Any other solution than yet one more macro?

[rriddle]

mlir/unittests/TableGen/PassGenTest.cpp

42

Well, I got something to compile in godbolt using friend functions:

https://godbolt.org/z/Yrf9Gzn34

[mscuttari]

Handle list option & Update documentation & Remove generated 'Pass' suffix

[mscuttari]

I've updated the documentation. Please give it a read and tell if it's not clear enough or if I missed something.
Also, I realized that the list options were not handled correctly. Now they generate an ArrayRef variable within the options struct. I've also added a list option within the test pass to check for the correct behaviour.

Edit: forgot to mention that I've implemented the emission of the constructors' definitions, based on the suggestion from @rriddle

mlir/include/mlir/Pass/PassBase.td

79–83

I would omit the diamond operator around PassName. With the new patches, the generated method would like create<PassName>() in the docs, which seems like a template specialization. createPassName() is clear enough imho.

[rriddle]

Looks good, thanks for working on this!

mlir/docs/PassManagement.md

882

mlir/tools/mlir-tblgen/PassGen.cpp

97–99

233–235

Do we always generate an Options struct? I assume we wouldn't in the case where the pass has no options.

345–353

Maybe a FIXME/TODO/or something to indicate that ideally this should be killed?

[mscuttari]

Address comments

[hpmorgan]

Hi there,

This change is triggering a warning (which we treat as error) in one of our builds:

/usr/bin/clang++ -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_INCLUDE_TESTS -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/__w/1/b/llvm/Release/tools/mlir/unittests/TableGen -I/__w/1/llvm-project/mlir/unittests/TableGen -I/__w/1/b/llvm/Release/include -I/__w/1/llvm-project/llvm/include -I/__w/1/llvm-project/mlir/include -I/__w/1/b/llvm/Release/tools/mlir/include -I/__w/1/llvm-project/mlir/unittests/TableGen/../../test/lib/Dialect/Test -I/__w/1/b/llvm/Release/tools/mlir/unittests/TableGen/../../test/lib/Dialect/Test -I/__w/1/llvm-project/llvm/utils/unittest/googletest/include -I/__w/1/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=mismatched-tags -O3 -DNDEBUG  -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -UNDEBUG -Wno-suggest-override -std=c++17 -MD -MT tools/mlir/unittests/TableGen/CMakeFiles/MLIRTableGenTests.dir/PassGenTest.cpp.o -MF tools/mlir/unittests/TableGen/CMakeFiles/MLIRTableGenTests.dir/PassGenTest.cpp.o.d -o tools/mlir/unittests/TableGen/CMakeFiles/MLIRTableGenTests.dir/PassGenTest.cpp.o -c /__w/1/llvm-project/mlir/unittests/TableGen/PassGenTest.cpp
In file included from /__w/1/llvm-project/mlir/unittests/TableGen/PassGenTest.cpp:12:
In file included from /__w/1/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58:
In file included from /__w/1/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:53:
In file included from /__w/1/llvm-project/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:50:
/__w/1/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1526:11: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
/__w/1/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1553:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<unsigned int, int>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
/__w/1/llvm-project/mlir/unittests/TableGen/PassGenTest.cpp:86:3: note: in instantiation of function template specialization 'testing::internal::EqHelper::Compare<unsigned int, int, nullptr>' requested here
  EXPECT_EQ(unwrap(pass)->getTestOption(), 57);
  ^
/__w/1/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2027:54: note: expanded from macro 'EXPECT_EQ'
  EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)

[mscuttari]

In D131839#3745687, @hpmorgan wrote:

Hi there,

This change is triggering a warning (which we treat as error) in one of our builds:

/usr/bin/clang++ -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_INCLUDE_TESTS -DMLIR_ROCM_CONVERSIONS_ENABLED=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/__w/1/b/llvm/Release/tools/mlir/unittests/TableGen -I/__w/1/llvm-project/mlir/unittests/TableGen -I/__w/1/b/llvm/Release/include -I/__w/1/llvm-project/llvm/include -I/__w/1/llvm-project/mlir/include -I/__w/1/b/llvm/Release/tools/mlir/include -I/__w/1/llvm-project/mlir/unittests/TableGen/../../test/lib/Dialect/Test -I/__w/1/b/llvm/Release/tools/mlir/unittests/TableGen/../../test/lib/Dialect/Test -I/__w/1/llvm-project/llvm/utils/unittest/googletest/include -I/__w/1/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=mismatched-tags -O3 -DNDEBUG  -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -UNDEBUG -Wno-suggest-override -std=c++17 -MD -MT tools/mlir/unittests/TableGen/CMakeFiles/MLIRTableGenTests.dir/PassGenTest.cpp.o -MF tools/mlir/unittests/TableGen/CMakeFiles/MLIRTableGenTests.dir/PassGenTest.cpp.o.d -o tools/mlir/unittests/TableGen/CMakeFiles/MLIRTableGenTests.dir/PassGenTest.cpp.o -c /__w/1/llvm-project/mlir/unittests/TableGen/PassGenTest.cpp
In file included from /__w/1/llvm-project/mlir/unittests/TableGen/PassGenTest.cpp:12:
In file included from /__w/1/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock.h:58:
In file included from /__w/1/llvm-project/llvm/utils/unittest/googlemock/include/gmock/gmock-actions.h:53:
In file included from /__w/1/llvm-project/llvm/utils/unittest/googlemock/include/gmock/internal/gmock-internal-utils.h:50:
/__w/1/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1526:11: error: comparison of integers of different signs: 'const unsigned int' and 'const int' [-Werror,-Wsign-compare]
  if (lhs == rhs) {
      ~~~ ^  ~~~
/__w/1/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:1553:12: note: in instantiation of function template specialization 'testing::internal::CmpHelperEQ<unsigned int, int>' requested here
    return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs);
           ^
/__w/1/llvm-project/mlir/unittests/TableGen/PassGenTest.cpp:86:3: note: in instantiation of function template specialization 'testing::internal::EqHelper::Compare<unsigned int, int, nullptr>' requested here
  EXPECT_EQ(unwrap(pass)->getTestOption(), 57);
  ^
/__w/1/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2027:54: note: expanded from macro 'EXPECT_EQ'
  EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)

Thanks for reporting. Fixed with d467515602fc55deddc910267f83c70c2e15f613.

mlir/tools/mlir-tblgen/PassGen.cpp

233–235

Fixed

[mscuttari]

@rriddle @mehdi_amini I'm updating my previous modifications to the existing passes but I've realized that the friend trick is not going to work, leading to undefined symbols at link time for what regard the createPassName methods. I suspect that the only solution is to split the GEN_PASS_DEF macro into something like GEN_PASS_DEF_BASECLASS_PASSNAME and GEN_PASS_DEF_CONSTRUCTOR_PASSNAME and include the .cpp.inc autogenerated file both before and after the actual class definition.
If that's ok, I will open a new review for this.

[mehdi_amini]

Can you send a draft revision that shows the issue?

[mscuttari]

In D131839#3746161, @mehdi_amini wrote:

Can you send a draft revision that shows the issue?

Sure, here it is: https://reviews.llvm.org/D132572

mlir/docs/PassManagement.md

def MyPass : Pass<"my-pass", "ModuleOp"> {
 
   // Specify any statistics.
   let statistics = [
     Statistic<"statistic", "example-statistic", "An example statistic">
   ];
 }
 ```
 
-Using the `gen-pass-decls` generator, we can generate most of the boilerplate
-above automatically. This generator takes as an input a `-name` parameter, that
+Using the `gen-pass-decls` and `gen-pass-defs` generators, we can generate most
+of the boilerplate above automatically.
+
+The `gen-pass-decls` generator takes as an input a `-name` parameter, that
 provides a tag for the group of passes that are being generated. This generator
-produces two chunks of output:
+produces code with two purposes:
 
-The first is a code block for registering the declarative passes with the global
-registry. For each pass, the generator produces a `registerFooPass` where `Foo`
-is the name of the definition specified in tablegen. It also generates a
-`registerGroupPasses`, where `Group` is the tag provided via the `-name` input
-parameter, that registers all of the passes present.
+The first is to register the declared passes with the global registry. For
+each pass, the generator produces a `registerPassName` where
+`PassName` is the name of the definition specified in tablegen. It also
+generates a `registerGroupPasses`, where `Group` is the tag provided via the
+`-name` input parameter, that registers all of the passes present.
 
 ```c++
 // gen-pass-decls -name="Example"
 
 #define GEN_PASS_REGISTRATION
 #include "Passes.h.inc"
 
 void registerMyPasses() {
   // Register all of the passes.
   registerExamplePasses();
 
   // Register `MyPass` specifically.
-  registerMyPassPass();
+  registerMyPass();
 }
 ```
 
-The second is a base class for each of the passes, containing most of the boiler
+The second is to provide a way to configure the pass options. These classes are
+named in the form of `MyPassOptions`, where `MyPass` is the name of the pass
+definition in tablegen. The configurable parameters reflect the options
+declared in the tablegen file. Differently from the registration hooks, these
+classes can be enabled on a per-pass basis by defining the
+`GEN_PASS_DECL_PASSNAME` macro, where `PASSNAME` is the uppercase version of
+the name specified in tablegen.
+
+```c++
+// .h.inc
+
+#ifdef GEN_PASS_DECL_MYPASS
+
+struct MyPassOptions {
+    bool option = true;
+    ::llvm::ArrayRef<int64_t> listOption;
+};
+
+#undef GEN_PASS_DECL_MYPASS
+#endif // GEN_PASS_DECL_MYPASS
+```
+
+If the `constructor` field has not been specified in the tablegen declaration,
+then autogenerated file will also contain the declarations of the default
+constructors.
+
+```c++
+// .h.inc
+
+#ifdef GEN_PASS_DECL_MYPASS
+...
+
+std::unique_ptr<::mlir::Pass> createMyPass();
+std::unique_ptr<::mlir::Pass> createMyPass(const MyPassOptions &options);
+
+#undef GEN_PASS_DECL_MYPASS
+#endif // GEN_PASS_DECL_MYPASS
+```
+
+The `gen-pass-defs` generator produces the definitions to be used for the pass
+implementation.
+
+It generates a base class for each of the passes, containing most of the boiler
 plate related to pass definitions. These classes are named in the form of
 `MyPassBase`, where `MyPass` is the name of the pass definition in tablegen. We
 can update the original C++ pass definition as so:
 
 ```c++
 /// Include the generated base pass class definitions.
-#define GEN_PASS_CLASSES
-#include "Passes.h.inc"
+#define GEN_PASS_DEF_MYPASS
+#include "Passes.cpp.inc"
 
 /// Define the main class as deriving from the generated base class.
 struct MyPass : MyPassBase<MyPass> {
   /// The explicit constructor is no longer explicitly necessary when defining
   /// pass options and statistics, the base class takes care of that
   /// automatically.
   ...
 
   /// The definitions of the options and statistics are now generated within
   /// the base class, but are accessible in the same way.
 };
-
-/// Expose this pass to the outside world.
-std::unique_ptr<Pass> foo::createMyPass() {
-  return std::make_unique<MyPass>();
-}
 ```
 
+Similarly to the previous generator, the definitions can be enabled on a
+per-pass basis by defining the appropriate preprocessor `GEN_PASS_DEF_PASSNAME`
+macro, with `PASSNAME` equal to the uppercase version of the name of the pass
+definition in tablegen.
+If the `constructor` field has not been specified in tablegen, then the default
+constructors are also defined and expect the name of the actual pass class to
+be equal to the name defined in tablegen.
+
 Using the `gen-pass-doc` generator, markdown documentation for each of the
 passes can be generated. See [Passes.md](Passes.md) for example output of real
 MLIR passes.
 
 ### Tablegen Specification
 
 The `Pass` class is used to begin a new pass definition. This class takes as an
 argument the registry argument to attribute to the pass, as well as an optional

mlir/include/mlir/Pass/PassBase.td

class PassBase<string passArg, string base> {
 
   // A short 1-line summary of the pass.
   string summary = "";
 
   // A human readable description of the pass.
   string description = "";
 
   // A C++ constructor call to create an instance of this pass.
-  code constructor = [{}];
+  // If empty, the default constructor declarations and definitions
+  // 'createPassName()' and 'createPassName(const PassNameOptions &options)'
+  // will be generated and the former will be used for the pass instantiation.
+  code constructor = "";
 
   // A list of dialects this pass may produce entities in.
   list<string> dependentDialects = [];
 
   // A set of options provided by this pass.
   list<Option> options = [];
 
   // A set of statistics provided by this pass.
   list<Statistic> statistics = [];

mlir/lib/TableGen/Pass.cpp

 
 StringRef Pass::getDescription() const {
   return def->getValueAsString("description");
 }
 
 StringRef Pass::getConstructor() const {
   return def->getValueAsString("constructor");
 }
+
 ArrayRef<StringRef> Pass::getDependentDialects() const {
   return dependentDialects;
 }
 
 ArrayRef<PassOption> Pass::getOptions() const { return options; }
 
 ArrayRef<PassStatistic> Pass::getStatistics() const { return statistics; }

mlir/tools/mlir-tblgen/PassCAPIGen.cpp

 //===----------------------------------------------------------------------===//
 
 void mlirRegister{0}Passes() {{
   register{0}Passes();
 }
 )";
 
 static bool emitCAPIImpl(const llvm::RecordKeeper &records, raw_ostream &os) {
   os << "/* Autogenerated by mlir-tblgen; don't manually edit. */";
   os << llvm::formatv(passGroupRegistrationCode, groupName);
 
   for (const auto *def : records.getAllDerivedDefinitions("PassBase")) {
     Pass pass(def);
     StringRef defName = pass.getDef()->getName();
-    os << llvm::formatv(passCreateDef, groupName, defName,
-                        pass.getConstructor());
+
+    std::string constructorCall;
+    if (StringRef constructor = pass.getConstructor(); !constructor.empty())
+      constructorCall = constructor.str();
+    else
+      constructorCall =
+          llvm::formatv("create{0}Pass()", pass.getDef()->getName()).str();
+
+    os << llvm::formatv(passCreateDef, groupName, defName, constructorCall);
   }
   return false;
 }
 
 static mlir::GenRegistration genCAPIHeader("gen-pass-capi-header",
                                            "Generate pass C API header",
                                            &emitCAPIHeader);
 
 static mlir::GenRegistration genCAPIImpl("gen-pass-capi-impl",
                                          "Generate pass C API implementation",
                                          &emitCAPIImpl);

mlir/tools/mlir-tblgen/PassGen.cpp

 using namespace mlir;
 using namespace mlir::tblgen;
 
 static llvm::cl::OptionCategory passGenCat("Options for -gen-pass-decls");
 static llvm::cl::opt<std::string>
     groupName("name", llvm::cl::desc("The name of this group of passes"),
               llvm::cl::cat(passGenCat));
 
+static void emitOldPassDecl(const Pass &pass, raw_ostream &os);
+
+/// Extract the list of passes from the TableGen records.
+static std::vector<Pass> getPasses(const llvm::RecordKeeper &recordKeeper) {
+  std::vector<Pass> passes;
+
+  for (const auto *def : recordKeeper.getAllDerivedDefinitions("PassBase"))
+    passes.emplace_back(def);
+
+  return passes;
+}
+
+const char *const passHeader = R"(
+//===----------------------------------------------------------------------===//
+// {0}
+//===----------------------------------------------------------------------===//
+)";
+
+//===----------------------------------------------------------------------===//
+// GEN: Pass registration generation
+//===----------------------------------------------------------------------===//
+
+/// The code snippet used to generate a pass registration.
+///
+/// {0}: The def name of the pass record.
+/// {1}: The pass constructor call.
+const char *const passRegistrationCode = R"(
+//===----------------------------------------------------------------------===//
+// {0} Registration
+//===----------------------------------------------------------------------===//
+
+inline void register{0}() {{
+  ::mlir::registerPass([]() -> std::unique_ptr<::mlir::Pass> {{
+    return {1};
+  });
+}
+
+// Old registration code, kept for temporary backwards compatibility.
+inline void register{0}Pass() {{
+  ::mlir::registerPass([]() -> std::unique_ptr<::mlir::Pass> {{
+    return {1};
+  });
+}
+)";
+
+/// The code snippet used to generate a function to register all passes in a
+/// group.
+///
+/// {0}: The name of the pass group.
+const char *const passGroupRegistrationCode = R"(
+//===----------------------------------------------------------------------===//
+// {0} Registration
+//===----------------------------------------------------------------------===//
+
+inline void register{0}Passes() {{
+)";
+
+/// Emits the definition of the struct to be used to control the pass options.
+static void emitPassOptionsStruct(const Pass &pass, raw_ostream &os) {
+  StringRef passName = pass.getDef()->getName();
+  ArrayRef<PassOption> options = pass.getOptions();
+
+  // Emit the struct only if the pass has at least one option.
+  if (options.empty())
+    return;
+
+  os << llvm::formatv("struct {0}Options {{\n", passName);
+
+  for (const PassOption &opt : options) {
+    std::string type = opt.getType().str();
+
+    if (opt.isListOption())
+      type = "::llvm::ArrayRef<" + type + ">";
+
+    os.indent(2) << llvm::formatv("{0} {1}", type, opt.getCppVariableName());
+
+    if (Optional<StringRef> defaultVal = opt.getDefaultValue())
+      os << " = " << defaultVal;
+
+    os << ";\n";
+  }
+
+  os << "};\n";
+}
+
+/// Emit the code to be included in the public header of the pass.
+static void emitPassDecls(const Pass &pass, raw_ostream &os) {
+  StringRef passName = pass.getDef()->getName();
+  std::string enableVarName = "GEN_PASS_DECL_" + passName.upper();
+
+  os << "#ifdef " << enableVarName << "\n";
+  os << llvm::formatv(passHeader, passName);
+
+  emitPassOptionsStruct(pass, os);
+
+  if (StringRef constructor = pass.getConstructor(); constructor.empty()) {
+    // Default constructor declaration.
+    os << "std::unique_ptr<::mlir::Pass> create" << passName << "();\n";
+
+    // Declaration of the constructor with options.
+    if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty())
+      os << llvm::formatv("std::unique_ptr<::mlir::Pass> create{0}(const "
+                          "{0}Options &options);\n",
+                          passName);
+  }
+
+  os << "#undef " << enableVarName << "\n";
+  os << "#endif // " << enableVarName << "\n";
+}
+
+/// Emit the code for registering each of the given passes with the global
+/// PassRegistry.
+static void emitRegistrations(llvm::ArrayRef<Pass> passes, raw_ostream &os) {
+  os << "#ifdef GEN_PASS_REGISTRATION\n";
+
+  for (const Pass &pass : passes) {
+    std::string constructorCall;
+    if (StringRef constructor = pass.getConstructor(); !constructor.empty())
+      constructorCall = constructor.str();
+    else
+      constructorCall =
+          llvm::formatv("create{0}()", pass.getDef()->getName()).str();
+
+    os << llvm::formatv(passRegistrationCode, pass.getDef()->getName(),
+                        constructorCall);
+  }
+
+  os << llvm::formatv(passGroupRegistrationCode, groupName);
+
+  for (const Pass &pass : passes)
+    os << "  register" << pass.getDef()->getName() << "();\n";
+
+  os << "}\n";
+  os << "#undef GEN_PASS_REGISTRATION\n";
+  os << "#endif // GEN_PASS_REGISTRATION\n";
+}
+
+static void emitDecls(const llvm::RecordKeeper &recordKeeper, raw_ostream &os) {
+  std::vector<Pass> passes = getPasses(recordKeeper);
+  os << "/* Autogenerated by mlir-tblgen; don't manually edit */\n";
+
+  for (const Pass &pass : passes)
+    emitPassDecls(pass, os);
+
+  emitRegistrations(passes, os);
+
+  // TODO drop old pass declarations
+  // Emit the old code until all the passes have switched to the new design.
+  os << "#ifdef GEN_PASS_CLASSES\n";
+  for (const Pass &pass : passes)
+    emitOldPassDecl(pass, os);
+  os << "#undef GEN_PASS_CLASSES\n";
+  os << "#endif // GEN_PASS_CLASSES\n";
+}
+
 //===----------------------------------------------------------------------===//
 // GEN: Pass base class generation
 //===----------------------------------------------------------------------===//
 
 /// The code snippet used to generate the start of a pass base class.
 ///
 /// {0}: The def name of the pass record.
 /// {1}: The base class for the pass.
 /// {2): The command line argument for the pass.
 /// {3}: The dependent dialects registration.
 const char *const passDeclBegin = R"(
-//===----------------------------------------------------------------------===//
-// {0}
-//===----------------------------------------------------------------------===//
-
 template <typename DerivedT>
 class {0}Base : public {1} {
 public:
   using Base = {0}Base;
 
   {0}Base() : {1}(::mlir::TypeID::get<DerivedT>()) {{}
   {0}Base(const {0}Base &other) : {1}(other) {{}
 
std::unique_ptr<::mlir::Pass> clonePass() const override {{
     return std::make_unique<DerivedT>(*static_cast<const DerivedT *>(this));
   }
 
   /// Return the dialect that must be loaded in the context before this pass.
   void getDependentDialects(::mlir::DialectRegistry &registry) const override {
     {4}
   }
 
   /// Explicitly declare the TypeID for this class. We declare an explicit private
   /// instantiation because Pass classes should only be visible by the current
   /// library.
   MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID({0}Base<DerivedT>)
 
-protected:
 )";
 
 /// Registration for a single dependent dialect, to be inserted for each
 /// dependent dialect in the `getDependentDialects` above.
 const char *const dialectRegistrationTemplate = R"(
   registry.insert<{0}>();
 )";
 
+const char *const friendDefaultConstructorTemplate = R"(
+  friend std::unique_ptr<::mlir::Pass> create{0}() {{
+    return std::make_unique<DerivedT>();
+  }
+)";
+
+const char *const friendDefaultConstructorWithOptionsTemplate = R"(
+  friend std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
+    return std::make_unique<DerivedT>(options);
+  }
+)";
+
 /// Emit the declarations for each of the pass options.
 static void emitPassOptionDecls(const Pass &pass, raw_ostream &os) {
   for (const PassOption &opt : pass.getOptions()) {
     os.indent(2) << "::mlir::Pass::"
                  << (opt.isListOption() ? "ListOption" : "Option");
 
     os << llvm::formatv(R"(<{0}> {1}{{*this, "{2}", ::llvm::cl::desc("{3}"))",
                         opt.getType(), opt.getCppVariableName(),

 static void emitPassStatisticDecls(const Pass &pass, raw_ostream &os) {
   for (const PassStatistic &stat : pass.getStatistics()) {
     os << llvm::formatv(
         "  ::mlir::Pass::Statistic {0}{{this, \"{1}\", \"{2}\"};\n",
         stat.getCppVariableName(), stat.getName(), stat.getDescription());
   }
 }
 
-static void emitPassDecl(const Pass &pass, raw_ostream &os) {
-  StringRef defName = pass.getDef()->getName();
+/// Emit the code to be used in the implementation of the pass.
+static void emitPassDefs(const Pass &pass, raw_ostream &os) {
+  StringRef passName = pass.getDef()->getName();
+  std::string enableVarName = "GEN_PASS_DEF_" + passName.upper();
+
+  os << "#ifdef " << enableVarName << "\n";
+  os << llvm::formatv(passHeader, passName);
+
   std::string dependentDialectRegistrations;
   {
     llvm::raw_string_ostream dialectsOs(dependentDialectRegistrations);
     for (StringRef dependentDialect : pass.getDependentDialects())
       dialectsOs << llvm::formatv(dialectRegistrationTemplate,
                                   dependentDialect);
   }
-  os << llvm::formatv(passDeclBegin, defName, pass.getBaseClass(),
+
+  os << llvm::formatv(passDeclBegin, passName, pass.getBaseClass(),
                       pass.getArgument(), pass.getSummary(),
                       dependentDialectRegistrations);
+
+  if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty()) {
+    os.indent(2) << llvm::formatv(
+        "{0}Base(const {0}Options &options) : {0}Base() {{\n", passName);
+
+    for (const PassOption &opt : pass.getOptions())
+      os.indent(4) << llvm::formatv("{0} = options.{0};\n",
+                                    opt.getCppVariableName());
+
+    os.indent(2) << "}\n";
+  }
+
+  // Protected content
+  os << "protected:\n";
   emitPassOptionDecls(pass, os);
   emitPassStatisticDecls(pass, os);
+
+  // Private content
+  os << "private:\n";
+
+  if (pass.getConstructor().empty()) {
+    os << llvm::formatv(friendDefaultConstructorTemplate, passName);
+
+    if (!pass.getOptions().empty())
+      os << llvm::formatv(friendDefaultConstructorWithOptionsTemplate,
+                          passName);
+  }
+
   os << "};\n";
+
+  os << "#undef " << enableVarName << "\n";
+  os << "#endif // " << enableVarName << "\n";
 }
 
-/// Emit the code for registering each of the given passes with the global
-/// PassRegistry.
-static void emitPassDecls(ArrayRef<Pass> passes, raw_ostream &os) {
-  os << "#ifdef GEN_PASS_CLASSES\n";
+static void emitDefs(const llvm::RecordKeeper &recordKeeper, raw_ostream &os) {
+  std::vector<Pass> passes = getPasses(recordKeeper);
+  os << "/* Autogenerated by mlir-tblgen; don't manually edit */\n";
+
   for (const Pass &pass : passes)
-    emitPassDecl(pass, os);
-  os << "#undef GEN_PASS_CLASSES\n";
-  os << "#endif // GEN_PASS_CLASSES\n";
+    emitPassDefs(pass, os);
 }
 
-//===----------------------------------------------------------------------===//
-// GEN: Pass registration generation
-//===----------------------------------------------------------------------===//
+// TODO drop old pass declarations
+// The old pass base class is being kept until all the passes have switched to
+// the new decls/defs design.
+const char *const oldPassDeclBegin = R"(
+template <typename DerivedT>
+class {0}Base : public {1} {
+public:
+  using Base = {0}Base;
 
-/// The code snippet used to generate a pass registration.
-///
-/// {0}: The def name of the pass record.
-/// {1}: The pass constructor call.
-const char *const passRegistrationCode = R"(
-//===----------------------------------------------------------------------===//
-// {0} Registration
-//===----------------------------------------------------------------------===//
+  {0}Base() : {1}(::mlir::TypeID::get<DerivedT>()) {{}
+  {0}Base(const {0}Base &other) : {1}(other) {{}
 
-inline void register{0}Pass() {{
-  ::mlir::registerPass([]() -> std::unique_ptr<::mlir::Pass> {{
-    return {1};
-  });
+  /// Returns the command-line argument attached to this pass.
+  static constexpr ::llvm::StringLiteral getArgumentName() {
+    return ::llvm::StringLiteral("{2}");
   }
-)";
+  ::llvm::StringRef getArgument() const override { return "{2}"; }
 
-/// The code snippet used to generate a function to register all passes in a
-/// group.
-///
-/// {0}: The name of the pass group.
-const char *const passGroupRegistrationCode = R"(
-//===----------------------------------------------------------------------===//
-// {0} Registration
-//===----------------------------------------------------------------------===//
+  ::llvm::StringRef getDescription() const override { return "{3}"; }
 
-inline void register{0}Passes() {{
-)";
+  /// Returns the derived pass name.
+  static constexpr ::llvm::StringLiteral getPassName() {
+    return ::llvm::StringLiteral("{0}");
+  }
+  ::llvm::StringRef getName() const override { return "{0}"; }
 
-/// Emit the code for registering each of the given passes with the global
-/// PassRegistry.
-static void emitRegistration(ArrayRef<Pass> passes, raw_ostream &os) {
-  os << "#ifdef GEN_PASS_REGISTRATION\n";
-  for (const Pass &pass : passes) {
-    os << llvm::formatv(passRegistrationCode, pass.getDef()->getName(),
-                        pass.getConstructor());
+  /// Support isa/dyn_cast functionality for the derived pass class.
+  static bool classof(const ::mlir::Pass *pass) {{
+    return pass->getTypeID() == ::mlir::TypeID::get<DerivedT>();
   }
 
-  os << llvm::formatv(passGroupRegistrationCode, groupName);
-  for (const Pass &pass : passes)
-    os << "  register" << pass.getDef()->getName() << "Pass();\n";
-  os << "}\n";
-  os << "#undef GEN_PASS_REGISTRATION\n";
-  os << "#endif // GEN_PASS_REGISTRATION\n";
+  /// A clone method to create a copy of this pass.
+  std::unique_ptr<::mlir::Pass> clonePass() const override {{
+    return std::make_unique<DerivedT>(*static_cast<const DerivedT *>(this));
   }
 
-//===----------------------------------------------------------------------===//
-// GEN: Registration hooks
-//===----------------------------------------------------------------------===//
+  /// Return the dialect that must be loaded in the context before this pass.
+  void getDependentDialects(::mlir::DialectRegistry &registry) const override {
+    {4}
+  }
 
-static void emitDecls(const llvm::RecordKeeper &recordKeeper, raw_ostream &os) {
-  os << "/* Autogenerated by mlir-tblgen; don't manually edit */\n";
-  std::vector<Pass> passes;
-  for (const auto *def : recordKeeper.getAllDerivedDefinitions("PassBase"))
-    passes.emplace_back(def);
+  /// Explicitly declare the TypeID for this class. We declare an explicit private
+  /// instantiation because Pass classes should only be visible by the current
+  /// library.
+  MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID({0}Base<DerivedT>)
 
-  emitPassDecls(passes, os);
-  emitRegistration(passes, os);
+protected:
+)";
+
+/// Emit a backward-compatible declaration of the pass base class.
+static void emitOldPassDecl(const Pass &pass, raw_ostream &os) {
+  StringRef defName = pass.getDef()->getName();
+  std::string dependentDialectRegistrations;
+  {
+    llvm::raw_string_ostream dialectsOs(dependentDialectRegistrations);
+    for (StringRef dependentDialect : pass.getDependentDialects())
+      dialectsOs << llvm::formatv(dialectRegistrationTemplate,
+                                  dependentDialect);
+  }
+  os << llvm::formatv(oldPassDeclBegin, defName, pass.getBaseClass(),
+                      pass.getArgument(), pass.getSummary(),
+                      dependentDialectRegistrations);
+  emitPassOptionDecls(pass, os);
+  emitPassStatisticDecls(pass, os);
+  os << "};\n";
 }
 
 static mlir::GenRegistration
-    genRegister("gen-pass-decls", "Generate pass declarations",
+    genPassDecls("gen-pass-decls", "Generate pass declarations",
                  [](const llvm::RecordKeeper &records, raw_ostream &os) {
                    emitDecls(records, os);
                    return false;
                  });
+
+static mlir::GenRegistration
+    genPassDefs("gen-pass-defs", "Generate pass definitions",
+                [](const llvm::RecordKeeper &records, raw_ostream &os) {
+                  emitDefs(records, os);
+                  return false;
+                });

mlir/unittests/TableGen/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS enums.td)
 mlir_tablegen(EnumsGenTest.h.inc -gen-enum-decls)
 mlir_tablegen(EnumsGenTest.cpp.inc -gen-enum-defs)
 add_public_tablegen_target(MLIRTableGenEnumsIncGen)
 
 set(LLVM_TARGET_DEFINITIONS passes.td)
 mlir_tablegen(PassGenTest.h.inc -gen-pass-decls -name TableGenTest)
+mlir_tablegen(PassGenTest.cpp.inc -gen-pass-defs -name TableGenTest)
 add_public_tablegen_target(MLIRTableGenTestPassIncGen)
 
 add_mlir_unittest(MLIRTableGenTests
   EnumsGenTest.cpp
   FormatTest.cpp
   OpBuildGen.cpp
   PassGenTest.cpp
 )

mlir/unittests/TableGen/PassGenTest.cpp

 //===- PassGenTest.cpp - TableGen PassGen Tests ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Pass/Pass.h"
+#include "llvm/ADT/STLExtras.h"
 
 #include "gmock/gmock.h"
 
-std::unique_ptr<mlir::Pass> createTestPass(int v = 0);
+std::unique_ptr<mlir::Pass> createTestPassWithCustomConstructor(int v = 0);
 
+#define GEN_PASS_DECL_TESTPASS
+#define GEN_PASS_DECL_TESTPASSWITHOPTIONS
+#define GEN_PASS_DECL_TESTPASSWITHCUSTOMCONSTRUCTOR
 #define GEN_PASS_REGISTRATION
 #include "PassGenTest.h.inc"
 
-#define GEN_PASS_CLASSES
-#include "PassGenTest.h.inc"
+#define GEN_PASS_DEF_TESTPASS
+#define GEN_PASS_DEF_TESTPASSWITHOPTIONS
+#define GEN_PASS_DEF_TESTPASSWITHCUSTOMCONSTRUCTOR
+#include "PassGenTest.cpp.inc"
 
 struct TestPass : public TestPassBase<TestPass> {
-  explicit TestPass(int v) : extraVal(v) {}
+  using TestPassBase::TestPassBase;
 
   void runOnOperation() override {}
 
   std::unique_ptr<mlir::Pass> clone() const {
     return TestPassBase<TestPass>::clone();
   }
-
-  int extraVal;
 };
 
-std::unique_ptr<mlir::Pass> createTestPass(int v) {
-  return std::make_unique<TestPass>(v);
+TEST(PassGenTest, defaultGeneratedConstructor) {
+  std::unique_ptr<mlir::Pass> pass = createTestPass();
+  EXPECT_TRUE(pass.get() != nullptr);
 }
 
 TEST(PassGenTest, PassClone) {
   mlir::MLIRContext context;
 
   const auto unwrap = [](const std::unique_ptr<mlir::Pass> &pass) {
     return static_cast<const TestPass *>(pass.get());
   };
 
-  const auto origPass = createTestPass(10);
+  const auto origPass = createTestPass();
+  const auto clonePass = unwrap(origPass)->clone();
+
+  EXPECT_TRUE(clonePass.get() != nullptr);
+  EXPECT_TRUE(origPass.get() != clonePass.get());
+}
+
+struct TestPassWithOptions
+    : public TestPassWithOptionsBase<TestPassWithOptions> {
+  using TestPassWithOptionsBase::TestPassWithOptionsBase;
+
+  void runOnOperation() override {}
+
+  std::unique_ptr<mlir::Pass> clone() const {
+    return TestPassWithOptionsBase<TestPassWithOptions>::clone();
+  }
+
+  unsigned getTestOption() const { return testOption; }
+
+  llvm::ArrayRef<int64_t> getTestListOption() const { return testListOption; }
+};
+
+TEST(PassGenTest, PassOptions) {
+  mlir::MLIRContext context;
+
+  TestPassWithOptionsOptions options;
+  options.testOption = 57;
+
+  llvm::SmallVector<int64_t, 2> testListOption = {1, 2};
+  options.testListOption = testListOption;
+
+  const auto unwrap = [](const std::unique_ptr<mlir::Pass> &pass) {
+    return static_cast<const TestPassWithOptions *>(pass.get());
+  };
+
+  const auto pass = createTestPassWithOptions(options);
+
+  EXPECT_EQ(unwrap(pass)->getTestOption(), 57);
+  EXPECT_EQ(unwrap(pass)->getTestListOption()[0], 1);
+  EXPECT_EQ(unwrap(pass)->getTestListOption()[1], 2);
+}
+
+struct TestPassWithCustomConstructor
+    : public TestPassWithCustomConstructorBase<TestPassWithCustomConstructor> {
+  explicit TestPassWithCustomConstructor(int v) : extraVal(v) {}
+
+  void runOnOperation() override {}
+
+  std::unique_ptr<mlir::Pass> clone() const {
+    return TestPassWithCustomConstructorBase<
+        TestPassWithCustomConstructor>::clone();
+  }
+
+  unsigned int extraVal = 23;
+};
+
+std::unique_ptr<mlir::Pass> createTestPassWithCustomConstructor(int v) {
+  return std::make_unique<TestPassWithCustomConstructor>(v);
+}
+
+TEST(PassGenTest, PassCloneWithCustomConstructor) {
+  mlir::MLIRContext context;
+
+  const auto unwrap = [](const std::unique_ptr<mlir::Pass> &pass) {
+    return static_cast<const TestPassWithCustomConstructor *>(pass.get());
+  };
+
+  const auto origPass = createTestPassWithCustomConstructor(10);
   const auto clonePass = unwrap(origPass)->clone();
 
   EXPECT_EQ(unwrap(origPass)->extraVal, unwrap(clonePass)->extraVal);
 }

mlir/unittests/TableGen/passes.td

 //===-- passes.td - PassGen test definition file -----------*- tablegen -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 include "mlir/Pass/PassBase.td"
 include "mlir/Pass/PassBase.td"
 include "mlir/Rewrite/PassUtil.td"
 
 def TestPass : Pass<"test"> {
   let summary = "Test pass";
+}
+
+def TestPassWithOptions : Pass<"test"> {
+  let summary = "Test pass with options";
+
+  let options = [
+    Option<"testOption", "testOption", "unsigned", "0", "Test option">,
+    ListOption<"testListOption", "test-list-option", "int64_t",
+               "Test list option">
+  ];
+}
 
-  let constructor = "::createTestPass()";
+def TestPassWithCustomConstructor : Pass<"test"> {
+  let summary = "Test pass with custom constructor";
 
-  let options = RewritePassUtils.options;
+  let constructor = "::createTestPassWithCustomConstructor()";
 }