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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.)
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.
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.
- Drop old declarations
- Fix some tests expecting the old pass names
- Convert missing flang passes
- Rebase to current upstream code
Fix formatting. Still I have to understand why clang-format doesn't run on some apparently random files.
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).
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).
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
I will revert and try to fix.
More conservative patch. Individual pass improvements are going to be handled by additional
patches.
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!
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.
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) |
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? |
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. |
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. |
mlir/tools/mlir-tblgen/PassGen.cpp | ||
---|---|---|
450 | Review is up: D133027 |
@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
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)