Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic infra required to move Numba to NewPassManager #1046

Merged
merged 28 commits into from
Jul 8, 2024

Conversation

yashssh
Copy link
Contributor

@yashssh yashssh commented May 2, 2024

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.

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.
ffi/Makefile.linux Outdated Show resolved Hide resolved
@gmarkall gmarkall self-requested a review May 7, 2024 14:40
@gmarkall gmarkall self-assigned this May 7, 2024
@gmarkall gmarkall added this to the v0.44.0 milestone May 7, 2024
@gmarkall
Copy link
Member

gmarkall commented May 9, 2024

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.

ffi/newpassmanagers.cpp Outdated Show resolved Hide resolved
@yashssh
Copy link
Contributor Author

yashssh commented May 9, 2024

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
Copy link
Member

gmarkall commented May 9, 2024

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.

Remove "new" prefix from new pass manager code

Move transforms.* code into legacypassmanagers.*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code moved to legacypassmanagers.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to legacypassmanagers.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code moved to legacypassmanagers.cpp

ffi/passmanagers.cpp Show resolved Hide resolved
@gmarkall
Copy link
Member

gmarkall commented May 14, 2024

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
Copy link
Contributor Author

yashssh commented May 16, 2024

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?

ffi/newpassmanagers.cpp Outdated Show resolved Hide resolved
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

API_EXPORT(LLVMFunctionPassManagerRef)
LLVMPY_buildFunctionSimplificationPipeline(LLVMPassBuilderRef PBref,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #1055 to further the discussion(s)

@@ -7,6 7,7 @@
from .linker import *
from .module import *
from .options import *
from .newpassmangers import *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

llvmlite/binding/newpassmangers.py Outdated Show resolved Hide resolved
yashssh and others added 2 commits May 31, 2024 17:23
Co-authored-by: Graham Markall <535640 [email protected]>
Co-authored-by: Graham Markall <535640 [email protected]>
@gmarkall
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gmarkall added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI labels Jun 4, 2024
@gmarkall
Copy link
Member

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

@sklam
Copy link
Member

sklam commented Jun 25, 2024

BFID: llvmlite_290

@sklam
Copy link
Member

sklam commented Jun 25, 2024

build passed

@sklam sklam added BuildFarm Passed For PRs that have been through the buildfarm and passed and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm labels Jun 25, 2024
@gmarkall
Copy link
Member

@sklam Many thanks! Can this go to RTM?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants