Add basic infra required to move Numba to NewPassManager

[yashssh]

Add separate classes and API endpoints for new pass manager and gradually plan on retiring/deleting legacy pass manger code.

This can be treated as a self contained starter patch to move Numba to NewPassManager. In following patches things like hooks for all other passes, refprune pass, more tests, TODOs and FIXMEs can be taken care of.

[gmarkall]

Just starting to look at this, but a general point - although the new pass manager is referred to as such in documentation, discussions, etc., to distinguish it from the legacy pass manager, can we avoid using "new" to refer to it in the code? There will be a time when the pass manager is not new, and then the name will be obsolete - instead can we just use the names like ModulePassManager, FunctionPassManager, PassBuilder, etc. where appropriate?

If we need to disambiguate between them in the names, I'd prefer if we renamed existing code to use the term "legacy" instead, where we can do so without exposing the change externally - for example, renaming passmanagers.cpp to legacypassmanagers.cpp, c.f. keeping the name PassManager for the Python legacy pass manager class.

[yashssh]

I agree with the naming convention comments. We should prefix the current pass manager files and classes with legacy prefix and remove new from the NewPassManager classes and files. I will have to check no current API breaks as a result of this change though.

[gmarkall]

Another note: With the changes in this PR and https://github.com/gmarkall/numba/tree/npm, I was able to test Numba with the new module pass manager (but not function pass manager). This resulted in:

Ran 11969 tests in 1314.257s

FAILED (failures=5, errors=3, skipped=607, expected failures=33)

from the testsuite, which is not too bad. I had to disable the refcount pruning pass, so some of the failures are about that. There are a few other failures I haven't looked into yet, but given the number of passing test cases I'm not too worried.

In #1043 I asked about the porting of the refcount pruning pass to the new pass manager on top of this PR, which may enable me to test more thoroughly.

[yashssh]

Code moved to legacypassmanagers.py

[yashssh]

Renamed to legacypassmanagers.py

[yashssh]

code moved to legacypassmanagers.cpp

[gmarkall]

A quick summary of changes / work needed towards getting this over the line, from OOB discussion:

• The example npm-usage.py should be in the examples dir, and tidied up / split up into multiple examples each demonstrating a bit less functionality than the entire example (does the example code need to be so large?)
• The documentation at https://llvmlite.readthedocs.io/en/latest/user-guide/binding/optimization-passes.html needs updating to reflect the new pass managers. Would you like me to work on this documentation? Let me know if so.
• Regarding the name newpassmanagers.cpp - lets leave it as it is, and minimise hassle. We can always rename it from newpassmanagers.cpp when we delete passmanagers.cpp.
• ffi/Makefile.linux - delete the removal of LTO flags, just keep this change locally.
• I need to review newpassmanagers.cpp and properly comprehend your answer to my earlier query.
• We need to find the minimal change that doesn't bake the word "new" into the public Python API for the new pass managers, nor change the existing API. (update: we need a little "new" in the API, it seems impossible to avoid)

[yashssh]

The example npm-usage.py should be in the examples dir, and tidied up / split up into multiple examples each demonstrating a bit less functionality than the entire example (does the example code need to be so large?)

Done

The documentation at https://llvmlite.readthedocs.io/en/latest/user-guide/binding/optimization-passes.html needs updating to reflect the new pass managers. Would you like me to work on this documentation? Let me know if so.

I tried adding it but please do add more/modify it if it isn't sufficient.

Regarding the name newpassmanagers.cpp - lets leave it as it is, and minimise hassle. We can always rename it from newpassmanagers.cpp when we delete passmanagers.cpp.

Sure

ffi/Makefile.linux - delete the removal of LTO flags, just keep this change locally.

Fixed

We need to find the minimal change that doesn't bake the word "new" into the public Python API for the new pass managers, nor change the existing API.

PassBuilder.getNewModulePassManager can be renamed to PassBuilder.getModulePassManager without consequences, same for the FPM api. But I don't see how we can get around removing new from the new classes as the names would clash with old Pass manager classes. Can we prefix the old PM classes with legacy prefix?

[gmarkall]

I accidentally started a review when I meant to add comments to the diff - no further comments now other than those on the diff.

[gmarkall]

Created #1055 to further the discussion(s)

[gmarkall]

Thanks - I think the old review comments will still be visible in the "conversation" tab, and I think I'm pretty happy with the resolution of the old comments, so feel free to rename it as soon as is convenient 🙂

[gmarkall]

Notes on my most recent testing with Numba, using the branch https://github.com/gmarkall/numba/tree/npm:

• Tested with the Module Pass Manager replaced to prove the concept for this branch (not the Function Pass Manager as well).
• Test results:

  Ran 11970 tests in 1489.362s
  
  FAILED (failures=19, errors=3, skipped=607, expected failures=33)
  

• Failures are not any cause for concern - reasons for them include:
  • No refop pruning with the new pass manager
  • My quick hack broke some caching
  • Some debuginfo is transformed under the new pass manager in a way that is not anticipated by the tests (regexes not matching, etc.)

In conclusion, I think this provides a sufficient base to develop Numba support for the New Pass Manager.

[gmarkall]

A quick further note related to my Numba testing branch - on some test runs there are no caching fails, in which case the results look like:

Ran 11970 tests in 1423.368s

FAILED (failures=8, errors=3, skipped=607, expected failures=33)

instead (i.e. 11 of the failures are from cache tests).

[gmarkall]

Thanks for all the efforts here! I think all review feedback is resolved now, and this provides a good base from which to start implementing NPM support in Numba.

(Note that the docs test is failing - it requires #1053)

[gmarkall]

I just merged main to get the changes from #1053, which should turn CI green.

[sklam]

BFID: llvmlite_290

[sklam]

build passed

[gmarkall]

@sklam Many thanks! Can this go to RTM?