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.

Sign up for GitHub

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

Jump to bottom

Move creation of mpm to optimize_final_module #9670

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Move creation of mpm to optimize_final_module #9670

merged 5 commits into from
Aug 5, 2024

Conversation

yashssh
Copy link
Contributor

@yashssh yashssh commented Jul 24, 2024

Fixes #9646

@stuartarchibald
Copy link
Contributor

Thanks for the patch @yashssh. Please could you add a release note to accompany this patch (instructions are here along with examples from other PRs in the directory above:

numba/docs/upcoming_changes/README.rst

Lines 1 to 47 in f3665c4

Changelog
=========
This directory contains "news fragments" which are short files that contain a
small **ReST**-formatted text that will be added to the next what's new page.
Make sure to use full sentences with correct case and punctuation, and please
try to use Sphinx intersphinx using backticks. The fragment should have a
header line and an underline using ``------`` followed by description of
your user-facing changes as they should appear in the release notes.
Each file should be named like ``<PULL REQUEST>.<TYPE>.rst``, where
``<PULL REQUEST>`` is a pull request number, and ``<TYPE>`` is one of:
* ``highlight``: Adds a highlighted bullet point to use as a possible highlight
of the release.
* ``np_support``: Addition of new NumPy functionality.
* ``deprecation``: Changes to existing code that will now emit a deprecation warning.
* ``expired``: Removal of a deprecated part of the API.
* ``compatibility``: A change which requires users to change code and is not
backwards compatible. (Not to be used for removal of deprecated features.)
* ``cuda``: Changes in the CUDA target implementation.
* ``new_feature``: New user facing features like ``kwargs``.
* ``improvement``: General improvements and edge-case changes which are
not new features or compatibility related.
* ``performance``: Performance changes that should not affect other behaviour.
* ``change``: Other changes
* ``doc``: Documentation related changes.
* ``infrastructure``: Infrastructure/CI related changes.
* ``bug_fix``: Bug Fixes for exiting features/functionality.
If you are unsure what pull request type to use, don't hesitate to ask in your
PR.
Once you've generated your fragment, to validate it, run
``python maint/towncrier_rst_validator.py --pull_request_id {PR Number}``
while on your branch and in numba base directory, where PR Number is the
assigned ID for your respective pull request on Github.
You can install ``towncrier`` and run ``towncrier build --draft``
if you want to get a preview of how your change will look in the final release
notes.
.. note::
This README was adapted from the NumPy changelog readme under the terms of
the `BSD-3 licence <https://github.com/numpy/numpy/blob/c1ffdbc0c29d48ece717acb5bfbf811c935b41f6/LICENSE.txt>`_.
)? I think this is an "improvement" type of change. Many thanks!

@stuartarchibald stuartarchibald added the 4 - Waiting on author Waiting for author to respond to review label Jul 26, 2024
stuartarchibald
stuartarchibald reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@stuartarchibald stuartarchibald 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 the patch and for investigating this @yashssh, there's a few minor things to resolve else looks good. It's great to see this issue fixed.

numba/core/codegen.py Outdated Show resolved Hide resolved
numba/tests/test_override.py Outdated Show resolved Hide resolved
numba/tests/test_override.py Outdated Show resolved Hide resolved
numba/tests/test_override.py Outdated
Comment on lines 42 to 54
instrs = [x for x in block.instructions if x.opcode != 'call']
op_expect = {'fadd', 'fmul', 'fdiv'}
started = False
for x in instrs:
if x.opcode in op_expect:
op_expect.remove(x.opcode)
if not started:
started = True
elif op_expect and started:
break

self.assertGreater(len(op_expect), 0,
"Function was optimized unexpectedly")
Copy link
Contributor

Choose a reason for hiding this comment

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

I've verified this manually against the IR produced on main and the IR produced with this PR. The IR produced when using this PR contains loads and stores interleaved with the fadd, fmul and fdiv instructions, whereas on main these loads and stores are optimised out and the arithmetic instructions appear sequentially.

@yashssh
Copy link
Contributor Author

yashssh commented Jul 29, 2024

Thanks for the review @stuartarchibald! I have added the release notes and addressed the comments.

@yashssh yashssh mentioned this pull request Jul 30, 2024
Open
@stuartarchibald
Copy link
Contributor

I'm just going to close and reopen this to trigger a rebuild on RTD now that #9675 resolved the issue.

@stuartarchibald
Copy link
Contributor

@yashssh I think that this branch is going to need to have numba:main merged in to pick up the changes in #9675 so that RTD will build.

@stuartarchibald stuartarchibald added this to the 0.61.0-rc1 milestone Jul 31, 2024
@yashssh
Copy link
Contributor Author

yashssh commented Jul 31, 2024

Are you suggesting to rebase my branch on top of main?

@stuartarchibald
Copy link
Contributor

Are you suggesting to rebase my branch on top of main?

I suggest merging HEAD (currently 99f5cc6) of numba:main into this branch. If you do a rebase all the commit SHAs change and github sometimes struggles to track comments/review across such a change, which can then result in having to do a re-review of the entire PR. Thanks!

@gmarkall gmarkall added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Aug 1, 2024
gmarkall
gmarkall approved these changes Aug 1, 2024
View reviewed changes
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.

The merge from main looks good.

@sklam sklam merged commit fb0caf6 into numba:main Aug 5, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartarchibald stuartarchibald stuartarchibald left review comments

@gmarkall gmarkall gmarkall approved these changes

Assignees
No one assigned
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Milestone
0.61.0-rc1
Development

Successfully merging this pull request may close these issues.

config.OPT is not overridden with override_config('OPT', 0)
4 participants
@yashssh @stuartarchibald @gmarkall @sklam