This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Update pass declarations to new autogenerated files
AbandonedPublic

Authored by mehdi_amini on Aug 29 2022, 2:06 AM.

Details

Summary

The patch introduces the required changes to update the pass declarations and definitions to use the new autogenerated files and allow dropping the old infrastructure.

Diff Detail

Event Timeline

mscuttari created this revision.Aug 29 2022, 2:06 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added 1 blocking reviewer(s): jpienaar. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
mscuttari requested review of this revision.Aug 29 2022, 2:06 AM

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

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?

mscuttari added a comment.EditedAug 29 2022, 2:12 AM

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?

PassDetail.h files are indeed not needed anymore and I removed them when it was trivial. There are some few passes that still need it, but it's just a matter of refactoring the code.

There's no real need for splitting .h.inc and .cpp.inc files, but it seems more clear to me from the user perspective. Public stuff goes into the .h.inc and gets included into the .h file, while private one goes into .cpp.inc and gets included by the .cpp one. Seems also coherent with the rest of the autogenerated code (e.g. dialect, types, etc.)

mscuttari updated this revision to Diff 456388.Aug 29 2022, 9:57 AM

Update flang passes

Herald added a project: Restricted Project. · View Herald Transcript
mehdi_amini accepted this revision.Aug 29 2022, 10:17 AM

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?

PassDetail.h files are indeed not needed anymore and I removed them when it was trivial. There are some few passes that still need it, but it's just a matter of refactoring the code.

There's no real need for splitting .h.inc and .cpp.inc files, but it seems more clear to me from the user perspective. Public stuff goes into the .h.inc and gets included into the .h file, while private one goes into .cpp.inc and gets included by the .cpp one. Seems also coherent with the rest of the autogenerated code (e.g. dialect, types, etc.)

I guess that would apply there as well: since we have macros anyway this is just extra boilerplate to generate more files and have more build steps / build configuration.

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?

PassDetail.h files are indeed not needed anymore and I removed them when it was trivial. There are some few passes that still need it, but it's just a matter of refactoring the code.

There's no real need for splitting .h.inc and .cpp.inc files, but it seems more clear to me from the user perspective. Public stuff goes into the .h.inc and gets included into the .h file, while private one goes into .cpp.inc and gets included by the .cpp one. Seems also coherent with the rest of the autogenerated code (e.g. dialect, types, etc.)

I guess that would apply there as well: since we have macros anyway this is just extra boilerplate to generate more files and have more build steps / build configuration.

I agree with Mehdi here.

rriddle accepted this revision.Aug 29 2022, 10:19 AM

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.

mscuttari updated this revision to Diff 456603.Aug 30 2022, 3:43 AM
  • Drop old declarations
  • Fix some tests expecting the old pass names
  • Convert missing flang passes
  • Rebase to current upstream code
mscuttari updated this revision to Diff 456622.Aug 30 2022, 5:15 AM

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

mscuttari added a comment.EditedAug 30 2022, 6:35 AM

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

rriddle accepted this revision.Aug 30 2022, 12:43 PM

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.

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.

Alright. I will wait also for @jpienaar (whose review seems to be mandatory from my understandings) and then make this land.

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.

Alright. I will wait also for @jpienaar (whose review seems to be mandatory from my understandings) and then make this land.

Ah, don't worry about that. You can feel free to land. (It's not mandatory in this case).

This revision is now accepted and ready to land.Aug 30 2022, 1:02 PM
This revision now requires review to proceed.Aug 30 2022, 1:02 PM

@jpienaar you can't escape!

Yes no need to wait for me here (I'm OOO but Rivers review sufficient for me here)

mscuttari updated this revision to Diff 456889.Aug 31 2022, 1:19 AM

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

mscuttari updated this revision to Diff 456905.Aug 31 2022, 2:19 AM

Fix formatting

mscuttari added a comment.EditedAug 31 2022, 4:36 AM

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)

P.S.: I can't close this patch, probably because of needed blocking review, but that's not a major issue atm.

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!

mscuttari added a comment.EditedAug 31 2022, 5:47 AM

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!

Here it is: https://github.com/llvm/llvm-project/issues/57475
I did not include a "before" section as it is very specific to the pass (i.e. dialect specific transforms have a different namespace, as I discussed in the issue). There is an example of how it should look like after the modifications though, I hope it is enough.

mehdi_amini added inline comments.Aug 31 2022, 7:37 AM
mlir/tools/mlir-tblgen/PassGen.cpp
450

Could we reintroduce this and give a depreciation notice? Like informing the community that they have x weeks to migrate?

(I thought you would just convert the passes in this patch and remove the backward compatible code in a subsequent change)

mscuttari added inline comments.Aug 31 2022, 7:41 AM
mlir/tools/mlir-tblgen/PassGen.cpp
450

Actually that was my plan, but I realized that the CAPI generated code was not compatible with the new one. It was not a big deal in previous patches as no pass was using the new infra and thus the CAPI part could still use the old naming convention, but I had to change it once we started relying on the new stuff. Should I reintroduce it anyway?

mehdi_amini added inline comments.Aug 31 2022, 7:44 AM
mlir/tools/mlir-tblgen/PassGen.cpp
450

If it is possible to reintroduce this so that not every single users of the pass infrastructure has to update right now, it would be great indeed.

mscuttari added inline comments.Aug 31 2022, 8:00 AM
mlir/tools/mlir-tblgen/PassGen.cpp
450

Well, I'm looking at the CAPI gen file (tools/mlir-tblgen/PassCAPIGen.cpp) and it should work anyway. There may have been a moment in the development process where it wasn't, and that idea persisted in my mind. I will make some more checks and upload a revision for this.

mscuttari added inline comments.Aug 31 2022, 8:19 AM
mlir/tools/mlir-tblgen/PassGen.cpp
450

Review is up: D133027
Notice that the registration code for C APIs is still incompatible with the new autogenerated code. The old one was appending "Pass", while we now assume that it's already there in the name of the pass. I honestly don't know how much sense it would have to also backward support that, we would need some option to be set, but then users would still have to add it to their code (at that point, they can just update to the new conventions).

mscuttari marked 2 inline comments as done.EditedSep 1 2022, 3:45 AM

@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

Mogball accepted this revision.Sep 6 2022, 8:32 AM

Just go ahead and land this

Just go ahead and land this

It's already landed :)
Can't close the revision though.

That's weird. I didn't realize that

This revision is now accepted and ready to land.Sep 6 2022, 8:37 AM
This revision now requires review to proceed.Sep 6 2022, 8:37 AM
Mogball resigned from this revision.Sep 6 2022, 8:38 AM
mehdi_amini commandeered this revision.Sep 6 2022, 8:54 AM
mehdi_amini abandoned this revision.
mehdi_amini edited reviewers, added: mscuttari; removed: mehdi_amini.
mlir/lib/Transforms/OpStats.cpp