This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mscuttari on Aug 13 2022, 8:26 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mscuttari requested review of this revision.Aug 13 2022, 8:26 AM
Mogball added inline comments.Aug 13 2022, 8:33 AM
mlir/tools/mlir-tblgen/PassGen.cpp
299

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

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 added inline comments.Aug 13 2022, 8:39 AM
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".

299

What do you mean with awkward?

mscuttari added inline comments.Aug 13 2022, 8:40 AM
mlir/tools/mlir-tblgen/PassGen.cpp
299

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 updated this revision to Diff 452450.EditedAug 13 2022, 1:00 PM

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 updated this revision to Diff 452662.Aug 15 2022, 7:29 AM

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 added inline comments.Aug 15 2022, 7:50 AM
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 added inline comments.Aug 15 2022, 7:59 AM
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 added inline comments.Aug 15 2022, 8:04 AM
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 added inline comments.Aug 15 2022, 8:07 AM
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

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 added inline comments.Aug 15 2022, 8:15 AM
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.

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 added inline comments.Aug 15 2022, 8:22 AM
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 added a comment.EditedAug 15 2022, 9:07 AM

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 added inline comments.Aug 15 2022, 9:29 AM
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 added inline comments.Aug 15 2022, 9:35 AM
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 requested changes to this revision.Aug 15 2022, 9:54 AM

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

This revision now requires changes to proceed.Aug 15 2022, 9:54 AM
Mogball added inline comments.Aug 15 2022, 9:56 AM
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

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 marked an inline comment as not done.EditedAug 15 2022, 1:06 PM

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 added inline comments.Aug 15 2022, 1:07 PM
mlir/include/mlir/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.h
23 ↗(On Diff #452662)

You will need to fix the flang build

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.

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 updated this revision to Diff 453114.EditedAug 16 2022, 1:45 PM

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 added a comment.EditedAug 16 2022, 1:49 PM

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)

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 added a comment.EditedAug 17 2022, 12:44 AM

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.

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 updated this revision to Diff 453240.Aug 17 2022, 2:34 AM

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

mscuttari updated this revision to Diff 453246.Aug 17 2022, 3:09 AM

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

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
354

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 added inline comments.Aug 17 2022, 10:01 AM
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

111

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?

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?

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

mscuttari marked 8 inline comments as done.

Address comments

mlir/tools/mlir-tblgen/PassGen.cpp
104

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

354

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.

mscuttari retitled this revision from [MLIR] Autogenerate struct and constructor to provide custom pass options to [MLIR] Split autogenerated pass declarations & C++ controllable pass options.Aug 17 2022, 10:47 AM
mscuttari edited the summary of this revision. (Show Details)

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!

Is there any doc to update?

mlir/tools/mlir-tblgen/PassGen.cpp
104

I"m fine with UPPER :)

mehdi_amini accepted this revision.Aug 17 2022, 1:31 PM

LG, but please wait for @Mogball to approve!

jpienaar accepted this revision.Aug 17 2022, 1:36 PM

Thanks!

mlir/tools/mlir-tblgen/PassGen.cpp
104

SGTM

rriddle requested changes to this revision.Aug 17 2022, 1:43 PM

This is missing documentation updates to PassManagement.md.

mlir/tools/mlir-tblgen/PassCAPIGen.cpp
110–112
mlir/tools/mlir-tblgen/PassGen.cpp
33

This should be a static method.

This revision now requires changes to proceed.Aug 17 2022, 1:43 PM
Mogball accepted this revision.Aug 17 2022, 1:46 PM

lgtm. Please address river's comments

mlir/include/mlir/Pass/PassBase.td
79–80
mlir/tools/mlir-tblgen/PassGen.cpp
354

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
43

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

This is missing documentation updates to PassManagement.md.

I proceed to update it.

mlir/tools/mlir-tblgen/PassCAPIGen.cpp
110–112

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 added inline comments.Aug 17 2022, 1:48 PM
mlir/tools/mlir-tblgen/PassCAPIGen.cpp
110–112

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
43

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 added inline comments.Aug 17 2022, 1:50 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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

mscuttari added inline comments.Aug 17 2022, 1:55 PM
mlir/tools/mlir-tblgen/PassCAPIGen.cpp
110–112

No problem, removing :)

mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 2:03 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 2:04 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

Remove "Base" suffix and done?

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

mscuttari added inline comments.Aug 17 2022, 2:41 PM
mlir/tools/mlir-tblgen/PassGen.cpp
354

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
43

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 added inline comments.Aug 17 2022, 2:46 PM
mlir/tools/mlir-tblgen/PassGen.cpp
354

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
43

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 added inline comments.Aug 17 2022, 2:51 PM
mlir/tools/mlir-tblgen/PassGen.cpp
354

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
43

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 added inline comments.Aug 17 2022, 2:52 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 2:53 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 3:03 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 3:07 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 3:27 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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 added inline comments.Aug 17 2022, 3:29 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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

rriddle added inline comments.Aug 17 2022, 3:49 PM
mlir/unittests/TableGen/PassGenTest.cpp
43

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

https://godbolt.org/z/Yrf9Gzn34

mscuttari updated this revision to Diff 453623.Aug 18 2022, 4:41 AM
mscuttari marked 13 inline comments as done.

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

mscuttari added a comment.EditedAug 18 2022, 4:44 AM

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–80

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.

mscuttari edited the summary of this revision. (Show Details)Aug 18 2022, 4:46 AM
rriddle accepted this revision.Aug 22 2022, 1:15 AM

Looks good, thanks for working on this!

mlir/docs/PassManagement.md
882 ↗(On Diff #453623)
mlir/tools/mlir-tblgen/PassGen.cpp
97–99
219–221

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

305–312

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

This revision is now accepted and ready to land.Aug 22 2022, 1:15 AM
mscuttari updated this revision to Diff 454960.Aug 23 2022, 2:20 PM
mscuttari marked 4 inline comments as done.

Address comments

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)

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
219–221

Fixed

mscuttari added a comment.EditedAug 24 2022, 9:41 AM

@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 added a comment.EditedAug 24 2022, 9:42 AM

Can you send a draft revision that shows the issue?

Can you send a draft revision that shows the issue?

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