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

Fix issue 4520 due to storage model mismatch #4623

Merged
merged 3 commits into from Oct 1, 2019

Conversation

sklam
Copy link
Member

@sklam sklam commented Sep 26, 2019

as titled.

@sklam sklam added this to the Numba 0.46 RC milestone Sep 26, 2019
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.

Looks good, thanks for the patch.

@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 3 - Ready for Review labels Sep 26, 2019
@esc
Copy link
Member

esc commented Sep 27, 2019

@sklam this is dark. Can you give us a --verbose on why this fixes the issue?

@esc esc closed this Sep 27, 2019
@esc esc reopened this Sep 27, 2019
@sklam
Copy link
Member Author

sklam commented Sep 27, 2019

@esc, yes, i'll try to explain this.

Background

Numba has this concept called datamodel (which I decided to call in storage model in the PR title because it is probably more descriptive). The datamodels describe how a type is represented in different situations:

  • value representation is for in-registers data or temporary in-stack data that does not need to be addressable.
  • data representation is for heap data that needs to be addressable.
  • argument and return representations need to be ABI compatible for the corresponding function boundary.

Most types have the same value and data representation but a bool is stored as a single bit in the value representation and a single byte in the data representation. The reason being that LLVM uses a i1 (1-bit integer) as the predicate for the branch instruction br, but a i1 is not addressable. The smaller addressable unit is a byte i8.

Related code: https://github.com/numba/numba/blob/master/numba/datamodel/models.py

The bug

This patch fixed the incref/decref wrapper function for the containers which is taking a pointer to the container elements using the data representation, but the underlying incref/decref operations are expecting the value representation. Elements in containers must be stored in the data representation because it needs to be addressable. To convert the data representation to value representation, the patch used the datamodel.load_from_data_pointer() instead of the builder.load().

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on CI Review etc done, waiting for CI to finish labels Sep 30, 2019
because str not supported on py2
@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 Sep 30, 2019
@stuartarchibald stuartarchibald added 4 - Waiting on CI Review etc done, waiting for CI to finish 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author 4 - Waiting on CI Review etc done, waiting for CI to finish labels Sep 30, 2019
@stuartarchibald stuartarchibald merged commit 7d0e3a6 into numba:master Oct 1, 2019
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.

None yet

3 participants