-
Notifications
You must be signed in to change notification settings - Fork 319
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
Conversation
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.
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 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 |
I agree with the naming convention comments. We should prefix the current pass manager files and classes with |
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:
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.*
llvmlite/binding/transforms.py
Outdated
There was a problem hiding this comment.
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
llvmlite/binding/passmanagers.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to legacypassmanagers.py
ffi/transforms.cpp
Outdated
There was a problem hiding this comment.
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
A quick summary of changes / work needed towards getting this over the line, from OOB discussion:
|
Done
I tried adding it but please do add more/modify it if it isn't sufficient.
Sure
Fixed
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 |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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)
llvmlite/binding/__init__.py
Outdated
@@ -7,6 7,7 @@ | |||
from .linker import * | |||
from .module import * | |||
from .options import * | |||
from .newpassmangers import * |
There was a problem hiding this comment.
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 🙂
Co-authored-by: Graham Markall <535640 [email protected]>
Co-authored-by: Graham Markall <535640 [email protected]>
Notes on my most recent testing with Numba, using the branch https://github.com/gmarkall/numba/tree/npm:
In conclusion, I think this provides a sufficient base to develop Numba support for the New Pass Manager. |
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:
instead (i.e. 11 of the failures are from cache tests). |
There was a problem hiding this 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)
I just merged |
BFID: |
build passed |
@sklam Many thanks! Can this go to RTM? |
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.