This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Patch for Support to loop bind clause : Checking Parent Region
AcceptedPublic

Authored by koops on Aug 18 2023, 3:01 AM.

Details

Summary

https://github.com/llvm/llvm-project/issues/64728
In https://reviews.llvm.org/D144634 : Support for Code Generation of loop bind clause,
before mapping a directive to the new directive there is a need to check if the parent directive/region is proper for the new mapped directive. e.g.
#pragma omp parallel for
for () {

#pragma omp loop bind(parallel)
for() {}

}.

After mapping "omp loop bind(parallel)" to "omp for" the above example becomes:
#pragma omp parallel for
for () {

#pragma omp for
for() {}

}.
However, it is illegal to have a "for" region within another "for" region. Thanks to David Pagan for pointing this out.

Diff Detail

Event Timeline

koops created this revision.Aug 18 2023, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2023, 3:01 AM
koops requested review of this revision.Aug 18 2023, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2023, 3:01 AM
ABataev added inline comments.Aug 18 2023, 5:05 AM
clang/include/clang/Basic/OpenMPKinds.h
251 ↗(On Diff #551446)

What if the outer regioun is sections?

Add tests for the nesting of regions to check all possible combinations correctly.

koops updated this revision to Diff 554920.Aug 31 2023, 12:26 AM

Using isOpenMPWorksharingDirective( ) for the "omp loop bind(parallel)" and "omp loop bind(teams)".
Added extra tests in loop_bind_messages.cpp.

jdoerfert retitled this revision from Patch for Support to loop bind clause : Checking Parent Region to [OpenMP] Patch for Support to loop bind clause : Checking Parent Region.Aug 31 2023, 6:15 PM
koops added a comment.Sep 6 2023, 9:43 PM

Can someone please review the changes that I uploaded last week?

Can someone please review the changes that I uploaded last week?

Still need to add extra tests to the nesting_of_regions.cpp test

koops updated this revision to Diff 557397.Sep 27 2023, 3:20 AM

Adding extra test cases to loop_bind_messages.cpp.

ABataev added inline comments.Sep 27 2023, 5:52 AM
clang/lib/Sema/SemaOpenMP.cpp
6167–6183

Can you move these checks to checkNestingOfRegions() function?

koops updated this revision to Diff 557506.Sep 30 2023, 4:11 AM

Moving checks of Work Sharing Directive in mapLoopConstruct to checkNestingOfRegions().

ABataev added inline comments.Oct 2 2023, 3:39 AM
clang/lib/Sema/SemaOpenMP.cpp
6223

Remove this new line

6238–6241

Can we do this before checks in lines 6232-6235 so we can avoid double call of checkNestingOfRegions for bind?

koops updated this revision to Diff 557639.EditedOct 7 2023, 3:20 AM

Avoiding executing "checkNestingOfRegions( )" multiple times.

ABataev added inline comments.Oct 9 2023, 2:39 PM
clang/lib/Sema/SemaOpenMP.cpp
6167–6176

Need to drop these extra calls of checkNestingOfRegions(), all check must be handled in the single call. Can we do it?

koops added inline comments.Oct 10 2023, 11:22 AM
clang/lib/Sema/SemaOpenMP.cpp
6167–6176
  1. Default binding in "#pragma omp loop" is handled in the beginning of mapLoopConstruct.

#pragma omp parallel
....
#pragma omp loop
...
indicates "omp loop" will be "omp loop bind(parallel)". So, I cannot call checkNestingOfRegions() before call to mapLoopConstruct()

  1. In mapLoopConstruct the directive/construct of "omp loop" is changed to other directives like "omp for". So, it is not possible to call checkNestingOfRegions() after the call to mapLoopConstruct.

Because of the above 2 reasons the current code seems correct. Please let me know if you have any alternative in mind.

ABataev added inline comments.Oct 10 2023, 11:36 AM
clang/lib/Sema/SemaOpenMP.cpp
6167–6176

Can you perform mapLoopConstruct earlier, before the very first checkNestingOfRegions()? Current code structure ovrcomplicates the code, makes it much harder to understand and to maintain

koops updated this revision to Diff 557876.Oct 25 2023, 3:36 AM

Removing multiple calls to Sema::checkNestingOfRegions() in Sema::ActOnOpenMPExecutableDirective() .

koops updated this revision to Diff 557877.Oct 25 2023, 4:37 AM
This revision is now accepted and ready to land.Oct 25 2023, 5:47 AM
koops added a comment.Oct 26 2023, 9:04 AM

Can someone please backout the changes done with this commit? It is resulting in build failures on debian.

koops reopened this revision.Oct 27 2023, 11:50 AM
This revision is now accepted and ready to land.Oct 27 2023, 11:50 AM

Did anything change after the patch was reverted?

Did anything change after the patch was reverted?

I have not changed anything in the code yet. I am still investigating. The test where it is failing has nothing to do with my changes. I do not understand the failure presently.

Ok. Thanks for the clarification.

Is there a github PR associated with this? If yes, what is the PR? If not, one should be created that includes a fix for the teams loop/loop error issue. Please add me as a reviewer. Thanks.