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 ind_to_const to enable fewer equivalence classes. #4981

Merged
merged 5 commits into from Dec 19, 2019

Conversation

DrTodd13
Copy link
Collaborator

Resolves #4963

@DrTodd13
Copy link
Collaborator Author

@stuartarchibald This patch seems to work for your test case and passes array analysis and parfor testing for me locally. This is a moderately scary change. Let me know what you think and how you want to progress.

@stuartarchibald
Copy link
Contributor

Thanks for the patch @DrTodd13, much appreciated.

This is a moderately scary change. Let me know what you think and how you want to progress.

What are you most worried about? Getting the const var scopes right (local vs global)?

@stuartarchibald stuartarchibald added this to the Numba 0.47 RC milestone Dec 18, 2019
@DrTodd13
Copy link
Collaborator Author

Thanks for the patch @DrTodd13, much appreciated.

This is a moderately scary change. Let me know what you think and how you want to progress.

What are you most worried about? Getting the const var scopes right (local vs global)?

I'm not worried about scoping. It is just a feeling about the degree of the change in combination with what triggered it which is new kinds of nodes being exposed in ways which previously weren't. Anytime you add some new data structure to this alias analysis code I think that is a significant change. I thought that might be a more isolated change than forcing both vars and consts into the ind_to_var dict.

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 fixing this, much appreciated. I understand your concerns over the addition of handling new node kinds, but unfortunately I don't think there's a equivalent/cheaper alternative available. I checked out and tested this locally, fix seems good. Only comment on the code is that it'd be good to add a test for freevars in the same circumstances (this triggers the same bug). Thanks again!

buf = np.zeros((_GLOBAL_INT_FOR_TESTING1, _GLOBAL_INT_FOR_TESTING2))
return buf
self.check(test_impl)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be good to also add a freevar variant:

    _FREEVAR_INT_FOR_TESTING1 = 17
    _FREEVAR_INT_FOR_TESTING2 = 5

    @skip_unsupported
    def test_issue4963_freevars(self):
        def test_impl():
            buf = np.zeros((_FREEVAR_INT_FOR_TESTING1, _FREEVAR_INT_FOR_TESTING2))
            return buf
        self.check(test_impl)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Dec 19, 2019
@stuartarchibald
Copy link
Contributor

@DrTodd13 Added IntelLabs#65 which implements the above, if you are happy with it please merge and this PR can be considered complete! Thanks.

@stuartarchibald stuartarchibald 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 Dec 19, 2019
@seibert seibert merged commit d51582a into numba:master Dec 19, 2019
@DrTodd13 DrTodd13 deleted the issue4963 branch February 10, 2020 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parfors issue with recent patch
3 participants