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

Undefined behaviour with tests/4.5/target_teams_distribute/test_target_teams_distribute_nowait.F90 #542

Open
tob2 opened this issue May 13, 2022 · 2 comments
Assignees

Comments

@tob2
Copy link
Collaborator

tob2 commented May 13, 2022

The testcase defines INTEGER(INT64),DIMENSION(N, N_TASKS):: work_storage, does not map work_storage and then:

https://github.com/SOLLVE/sollve_vv/blob/ea88f50028a5e56709ca1b4467e05c4689a374fb/tests/4.5/target_teams_distribute/test_target_teams_distribute_nowait.F90#L45-L46

Namely, it runs asynchronous and maps a disjunct array sections/storage of the array.

When run asynchronously, I do see fails here when the unmapping happens for one target region while another is still running.

I believe this violates the following wording of OpenMP 5.1 (= 5.2), "Restrictions to the map clause are as follows:"

  • If an array appears as a list item in a map clause, multiple parts of the array have corresponding storage in the device data environment prior to a task encountering the construct associated with the map clause, and the corresponding storage for those parts was created by maps from more than one earlier construct, the behavior is unspecified.

Thus:

  • I belief that "array" refers in this case to the containing array, i.e. to the whole of work_storage – and not to the array item / array section work_storage(1:N, x).

  • I note that the wording was added for 5.1 and the testcase is for 4.5, but I read the 5.1 wording as clarification/correction and not as a non-backwards compatible change.

  • For completeness, the 5.1 wording was added for the non-publicly accessible OpenMP Spec Issue 1909

Do you concur that the wording above makes the testcase invalid? If not/if unsure, we could ask the experts.

@nolanbaker31 @tmh97 @spophale – thoughts?

@tob2
Copy link
Collaborator Author

tob2 commented May 13, 2022

First, the following loop is here only executed concurrently if I wrap the DO loop in, e.g. !$omp parallel do:
https://github.com/SOLLVE/sollve_vv/blob/ea88f50028a5e56709ca1b4467e05c4689a374fb/tests/4.5/target_teams_distribute/test_target_teams_distribute_nowait.F90#L45-L46
otherwise, was_async is always false.
https://github.com/SOLLVE/sollve_vv/blob/ea88f50028a5e56709ca1b4467e05c4689a374fb/tests/4.5/target_teams_distribute/test_target_teams_distribute_nowait.F90#L70-L74

I wonder whether any compiler will run the target regions concurrently without. → Consider to run in a parallel-do construct.

Secondly, we discussed this internally – result:

  • pedantically the testcase is invalid (for the reasons/quote above)
  • practically it should work fine — at least if either the implementation does not use an array descriptor or handles the descriptor itself (and not the pointer in it) as firstprivate.

Thus, you may consider to keep the testcase, despite the pedantic issue. Or you fix it.

@spophale
Copy link
Collaborator

@tob2
I don't agree that the test is invalid, unless I am missing something, there is no attempt to map the (whole) array after the portions of the arrays are mapped in the loop. But I do agree to the suggestion posted on May 13th, the do loop needs to be an omp parallel do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants