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 #5570. Incorrect race variable detection due to SSA naming. #5630

Merged
merged 2 commits into from Apr 30, 2020

Conversation

sklam
Copy link
Member

@sklam sklam commented Apr 24, 2020

Fixes #5570

@sklam sklam changed the title Fix #5570. Discard race variables that are referring to unversioned n… Fix #5570. Incorrect race variable detection due to SSA naming. Apr 24, 2020
@sklam sklam added this to the 0.49.1 milestone Apr 24, 2020
@sklam sklam marked this pull request as ready for review April 27, 2020 18:41
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. Few minor things to address else looks good.

Comment on lines +3193 to +3194
except ir.NotDefinedError:
# XXX: it's a bug that something references an undefined name
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this actually just raise?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, some passes are not registering names into scope. There are other similar situation where we just ignore the NotDefinedError in parfor passes

# XXX: it's a bug that something references an undefined name
return k

keep = set(races)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps s/keep/races1/ so as to match up with the docstring?


keep = set(races)
for rv in races:
if any(unversion(rv) == unversion(pv) for pv in params):
Copy link
Contributor

Choose a reason for hiding this comment

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

could the unversion(pv) for pv in params be hoisted as loop invariant?

@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Apr 29, 2020
@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 Apr 29, 2020
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 fix-ups, looks good.

@stuartarchibald stuartarchibald added 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 labels Apr 30, 2020
@stuartarchibald
Copy link
Contributor

/AzurePipelines run

@azure-pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@stuartarchibald stuartarchibald mentioned this pull request Apr 30, 2020
40 tasks
@sklam sklam merged commit 47cdbe5 into numba:master Apr 30, 2020
@sklam sklam deleted the fix/iss5570 branch April 30, 2020 18:34
sklam added a commit that referenced this pull request Apr 30, 2020
Fix #5570. Incorrect race variable detection due to SSA naming.
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.

Datashader/Numba various LoweringErrors
2 participants