Page MenuHomePhabricator

[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
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

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?

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

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.

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
101–103
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–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.

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 added inline comments.Aug 17 2022, 1:48 PM
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 added inline comments.Aug 17 2022, 1:50 PM
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 added inline comments.Aug 17 2022, 1:55 PM
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 added inline comments.Aug 17 2022, 2:03 PM
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 added inline comments.Aug 17 2022, 2:04 PM
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 added inline comments.Aug 17 2022, 2:41 PM
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 added inline comments.Aug 17 2022, 2:46 PM
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 added inline comments.Aug 17 2022, 2:51 PM
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 added inline comments.Aug 17 2022, 2:52 PM
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 added inline comments.Aug 17 2022, 2:53 PM
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 added inline comments.Aug 17 2022, 3:03 PM
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 added inline comments.Aug 17 2022, 3:07 PM
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 added inline comments.Aug 17 2022, 3:27 PM
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 added inline comments.Aug 17 2022, 3:29 PM
mlir/unittests/TableGen/PassGenTest.cpp
42

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
42

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

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
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?

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
233–235

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