Thanks for the patch. I am away from home though, I will try giving a look next week!

In D134766#3821105, @rkayaith wrote:

Might be worth adding for consistency, but I don't have a use case for that at the moment. Typically I see the pass definitions for a group being spread across multiple files.

LGTM. What about doing this also for the definitions?

Easier constructor inheritance

Thanks for addressing the issue

In D132838#3772159, @Mogball wrote:

Just go ahead and land this

@jpienaar when you are back, please accept the review so that I can then hopefully close it (no hurry, take your time :))
Part of the non-auto-closing issue is that I wrongly put by hand "Differential Review" instead of "Differential Revision", but still I have no option to close it manually

In D133027#3762562, @mehdi_amini wrote:

Thanks! Can you start a thread (tagged [PSA]) on discord announcing the deprecation and including the instructions for downstream to upgrade within X weeks? (anything between 2 to 6 weeks is likely fine)

In D132838#3760825, @mehdi_amini wrote:

What should I do for the remaining ones (basically, all)? @rriddle was mentioning to open bugs and delegate them, but how should I proceed? One bug for each pass seems a lot to be done manually and would clutter the github issue page.

In general we operate per areas: that is it could be "per dialect". I would file a single bug for the entire project and then tag people for each as we go.
If you file the bug with the instructions (before/after and pointer to the doc) that would already be great!

Patch landed, all builds seem fine.
As I have interest in some specific passes for my projects, I will upload additional revisions updating their code to be coherent with what explained in the docs (that is, they need naming change and options usage).
What should I do for the remaining ones (basically, all)? @rriddle was mentioning to open bugs and delegate them, but how should I proceed? One bug for each pass seems a lot to be done manually and would clutter the github issue page. Maybe we should let people discover the change and wait for a volunteer to fix his area of interest? (joking, but not too much :D)

Fix formatting

More conservative patch. Individual pass improvements are going to be handled by additional
patches.

Well, build bots are complaining about some problems.
https://lab.llvm.org/buildbot/#/builders/220/builds/4537
https://lab.llvm.org/buildbot/#/builders/61/builds/31581

In D132838#3759415, @rriddle wrote:

Don't feel like you are pressured to do all of the follow up cleanups, please file bugs for things and we can delegate from there.

Patch seems good from the build bots perspective. I've also dropped the old declarations, as they would not be needed anymore after this patch.
A lot of passes can be further updated to take advantage of the new infra (i.e. inheriting constructors, accepting options, removing redundant option structures), but I would like to separate this into additional and more self-contained patches, and possibly delegate them to more people as they need them to be implemented. My idea behind this patch is to just drop the old autogenerated files and adopt the new ones (patching the whole codebase to reflect the best practices is too much for a single person and sometimes would require discussing names, code organization, etc. which seems out of scope now).

Fix formatting. Still I have to understand why clang-format doesn't run on some apparently random files.

• Drop old declarations
• Fix some tests expecting the old pass names
• Convert missing flang passes
• Rebase to current upstream code

Fix formatting

Fix formatting

Right. I will land D132572, open a new review to merge .cpp.inc into .h.inc (just for passes, fixing all the infra also for types, etc. is a bit too much for me) and then update this patch.

Update flang passes

In D132838#3754956, @mehdi_amini wrote:

Nice!

I love that we remove the need for PassDetail.h.

Since we guard things behind macros, could we generate everything in Passes.h.inc instead of the need of a separate TableGen invocation for Passes.cpp.inc? Is there an advantage to use both?

This patch follows D131839 and D132572. Changes of D132572 are also included in this patch, but they will disappear once that patch will land.

I've updated the docs. Feel free to comment on them if you feel them not to be clear enough.
In the meanwhile, I've uploaded a first draft regarding the patches to the pass declarations, so we can start discussing about them: D132838

Update docs

@rriddle I'm sorry for the early ping, but I'm fixing the big patch regarding all the passes and I would like to know if the current namespace separation looks good. I'd like to avoid reiterating over all the passes just for this :D

impl namespace for pass base class and friend create constructors

In D132572#3751139, @rriddle wrote:

Nice! Do you think we should add a namespace field to the tablegen pass def now? Seems like that would be consistent with everything else (and would remove potential user foot guns as well).

@rriddle thanks for the suggestion, working flawlessly. I have uploaded your fix and removed the changes I was doing to the passes, which I will instead upload in a separate patch in order to avoid a single commit mixing different stuff.

Add createPassNameImpl methods

In D132572#3747742, @mehdi_amini wrote:

The issue is that the friend function in the class gets an inline linkage: it only gets emitted if it used in the translation unit.
I'm not sure if it can be made "not inline", usually you have to define it outside of the class but I don't know what the syntax would be here...

Wrapping the base class in mlir namespace. It is indeed the correct way to go, but still the
problem persists.

In D131839#3746161, @mehdi_amini wrote:

Can you send a draft revision that shows the issue?

Compilation log (a bit trimmed, for clearness):

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

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)

Address comments

Closed by commit 8a10ee7590a91c9b2e5c80b52822a3a4c3af1a15
Forgot to add the review link this time :(

Removed wrong and useless explicit copy constructor from DynamicMemRefType

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.

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

In D131839#3730050, @rriddle wrote:

This is missing documentation updates to PassManagement.md.

Address comments

@tjoerg Can you please explain how to reproduce the problem? Running the tests with the following cmake configuration doesn't make them failing: cmake -DENABLE_PROJECTS="clang;mlir;compiler-rt" -DLLVM_USE_SANITIZER="Undefined"

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

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

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

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)

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.

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.

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.

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.