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

Use get_parfor_writes to detect illegal array access that prevents fusion. #7582

Merged
merged 17 commits into from Aug 2, 2023

Conversation

DrTodd13
Copy link
Collaborator

Resolves #7578

In parfor fusion, there is a check that something written in one parfor not be used in the second parfor. There was a case in issue #7578 that an array was written in the first parfor and analysis of that parfor said that the array was "used" but not written in that parfor. Therefore, the second parfor which also used the same array didn't see a conflict and did the fusion. If you just make the array written in the first parfor loop body as a "def" then the array isn't passed to the parfor gufunc and ends up undefined. So, what I am trying here is to make the array both def and use so that the parfors don't fuse but the array is passed as a parameter to the gufunc.

This is in-progress at the moment as I expect this to break legitimate instances of fusion because the real problem with this example is that indices outside of the parfor index are used in the second parfor which is the only thing that should prevent fusion in this case. I want to see what the tests say to see which tests fail due to this and if none do then to create one that should be allowed.

…then make sure that the second parfor only accesses that array using the parfor index variable. Since fusion is predicated on the loop index variables being the same (size), this is a cheap test for there being no dependencies.
@gmarkall gmarkall added this to the Numba 0.56 RC milestone Nov 19, 2021
@ehsantn
Copy link
Collaborator

ehsantn commented Nov 22, 2021

Can something like get_stmt_writes be used instead? Use/def and liveness are used in many places and messing with them is potentially very dangerous. Setitem doesn't really have def semantics.

@DrTodd13 DrTodd13 changed the title Make SetItem array both def and use. Use get_parfor_writes to detect illegal array access that prevents fusion. Nov 23, 2021
@DrTodd13
Copy link
Collaborator Author

Can something like get_stmt_writes be used instead? Use/def and liveness are used in many places and messing with them is potentially very dangerous. Setitem doesn't really have def semantics.

I did something similar now. Please have a look.

@ehsantn
Copy link
Collaborator

ehsantn commented Nov 26, 2021

Didn't go through the details but seems like this is on the right track. Thanks.

@sklam sklam modified the milestones: Numba 0.56 RC, Numba 0.57 RC Jun 1, 2022
@sklam sklam self-assigned this Jul 19, 2022
@esc esc modified the milestones: Numba 0.57 RC, Numba 0.58 RC Jun 27, 2023
overlap = p1_body_defs.intersection(p2_uses)
# overlap are those variable defined in first parfor and used in the second
if len(overlap) != 0:
is_safe = True # Assume safe until proven otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Can it default to be unsafe instead for a more conservative behavior?

It seems it just need all overlap variables to be of ArrayCompatible and in p2arraynotindexed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can do the following:

unsafe_var = [not isinstance(typemap[x], types.ArrayCompatible) or x in p2arraynotindexed for x in overlap]
if any(unsafe_var):

That is simpler looking but this does have potential to be slower than what I wrote that gives up as soon as it finds the first problem. The new line above would potentially compute some useless stuff but the amount of work is trivial so I've checked in this change. See if you like it.

Choose a reason for hiding this comment

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

Would a generator comprehension solve this problem? If you change the brackets to parentheses, then the body will be lazily evaluated by any(): unsafe_var = (not isinstance(typemap[x], types.ArrayCompatible) or x in p2arraynotindexed for x in overlap)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Done.

@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jun 29, 2023
@esc
Copy link
Member

esc commented Jul 12, 2023

towncrier support was fixed with #9069

I think you'll need to merge in main to get towncrier to work again with the patches on main.

@esc
Copy link
Member

esc commented Jul 12, 2023

Closing to open again in an attempt to re-trigger towncrier. It should "just work" on this PR.

@esc esc closed this Jul 12, 2023
@esc esc reopened this Jul 12, 2023
@esc
Copy link
Member

esc commented Jul 12, 2023

@DrTodd13 there is a bug in the filename for towncrier: 7582.big_fix.rst should be 7582.bug_fix.rst, can you fix?

@stuartarchibald stuartarchibald 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 Jul 12, 2023
sklam
sklam previously approved these changes Jul 12, 2023
Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

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

LGTM

@sklam sklam 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 Jul 12, 2023
@esc esc 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 Jul 13, 2023
@esc
Copy link
Member

esc commented Jul 13, 2023

@DrTodd13 thank you for the patch. @sklam thank you for the review. I have marked this as RTM now.

@esc esc added ParallelAccelerator 4 - Waiting on author Waiting for author to respond to review and removed 5 - Ready to merge Review and testing done, is ready to merge labels Jul 13, 2023
@esc esc unassigned sklam Jul 14, 2023
@esc
Copy link
Member

esc commented Jul 14, 2023

@DrTodd13 after merging #9037 this now conflicts with main.

@DrTodd13 DrTodd13 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 Jul 14, 2023
@DrTodd13
Copy link
Collaborator Author

@DrTodd13 after merging #9037 this now conflicts with main.

Resolved.

@sklam
Copy link
Member

sklam commented Jul 15, 2023

(CI failed due to conda timeout. i've restarted failing builds)

@sklam sklam 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 Aug 1, 2023
@esc esc merged commit fcf9420 into numba:main Aug 2, 2023
23 checks passed
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 ParallelAccelerator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation with parallel=True gives garbage results for a very simply parallelizable function
7 participants