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

Fix #6444: pruner issues with reference stealing functions #6446

Merged
merged 8 commits into from Nov 10, 2020
Merged

Fix #6444: pruner issues with reference stealing functions #6446

merged 8 commits into from Nov 10, 2020

Conversation

sklam
Copy link
Member

@sklam sklam commented Nov 2, 2020 •
edited

Fixes #6444

@sklam sklam added this to the 0.52.0RC3 milestone Nov 2, 2020
@sklam sklam marked this pull request as ready for review November 3, 2020 15:58
stuartarchibald
stuartarchibald reviewed Nov 4, 2020
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, think this will fix the issue. Is there a test that can be added to demonstrate?

docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated


Quirks
------
Quirks of old optimization pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Qualify "old" again? Also perhaps this should be called "legacy"?

numba/core/runtime/_nrt_python.c Show resolved Hide resolved
numba/core/runtime/_nrt_python.c Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Nov 4, 2020
sklam and others added 3 commits November 5, 2020 12:37
@sklam @stuartarchibald
Apply suggestions from code review
441d8cd 
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@sklam
Clarify old and new refprune
5b1d386
@sklam
Add comments for NRT_adapt_ndarray_to_python_acqref
ffb867b 
about the renaming
@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 5, 2020
@stuartarchibald
Copy link
Contributor

stuartarchibald commented Nov 6, 2020 •
edited

Close/reopen was because Azure got irretrievably stuck due to a deprovisioning request.

stuartarchibald
stuartarchibald reviewed Nov 6, 2020
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 fixes, a typo and a query else looks good.

numba/core/runtime/_nrt_python.c Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated
Comment on lines 80 to 83
Since the pre-0.52.0 `refcount optimization pass <nrt-refct-opt-pass_>`_
requires LLVM function optimization pass, the pass works on the LLVM IR as
text. The optimized IR is then materialized again as a new LLVM in-memory
bitcode object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this is trying to convey? Is it that the old pass works on textual IR, so the bitcode repr has to be optimised by LLVM whilst as bitcode, then converted to text, optimisations run on this text and then materialised back to bitcode?

Copy link
Member Author

@sklam sklam Nov 6, 2020 •
edited

Choose a reason for hiding this comment

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

Yes. I agree it wasn't written clearly. I didn't actually change it though, just shuffled the line-breaks around to fit.
(the lazy part of me doesn't want to change it since it's going away very soon)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on the basis that only core developers adjusted it and it's being retired. Apart from adding the missing "the" above, let's just leave it.

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Nov 6, 2020
@sklam @stuartarchibald
Update numba/core/runtime/_nrt_python.c
3188584 
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
stuartarchibald
stuartarchibald reviewed Nov 9, 2020
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.

Few typos else looks good, thanks for the fixes.

docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated Show resolved Hide resolved
docs/source/developer/numba-runtime.rst Outdated
Comment on lines 80 to 83
Since the pre-0.52.0 `refcount optimization pass <nrt-refct-opt-pass_>`_
requires LLVM function optimization pass, the pass works on the LLVM IR as
text. The optimized IR is then materialized again as a new LLVM in-memory
bitcode object.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on the basis that only core developers adjusted it and it's being retired. Apart from adding the missing "the" above, let's just leave it.

@sklam @stuartarchibald
Apply suggestions from code review
eb596fb 
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 9, 2020
@stuartarchibald
Copy link
Contributor

13b9569 fixes a typo.

@stuartarchibald
Copy link
Contributor

Assuming CI passes this is good to go, thanks for fixing this!

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Nov 10, 2020
@stuartarchibald
Copy link
Contributor

/AzurePipelines run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Nov 10, 2020
@sklam sklam merged commit fa9b152 into numba:master Nov 10, 2020
sklam added a commit to sklam/numba that referenced this pull request Nov 12, 2020
@sklam
Merge pull request numba#6446 from sklam/fix/iss6444
a3ee4b6 
Fix numba#6444: pruner issues with reference stealing functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartarchibald stuartarchibald stuartarchibald 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.52.0RC3
Development

Successfully merging this pull request may close these issues.

0.52.0rc2 regression: invalid array data if used after print
2 participants
@sklam @stuartarchibald