Page MenuHomePhabricator

[flang][OpenMP] Add semantic checks for occurrence of nested Barrier regions
ClosedPublic

Authored by arnamoy10 on Apr 5 2021, 9:38 AM.

Details

Summary

This patch adds the following nesting check for barrier constructs:

A barrier region may not be closely nested inside a worksharing, loop, task, taskloop, critical, ordered, atomic, or master region.

Also adds a test case for the check,

Diff Detail

Event Timeline

arnamoy10 created this revision.Apr 5 2021, 9:38 AM
arnamoy10 requested review of this revision.Apr 5 2021, 9:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
flang/lib/Semantics/check-omp-structure.cpp
808

Definition of closely nested region is
"A region nested inside another region with no parallel region nested between them."

Does this check capture this definition correctly?

815–828

Nit: Remove commented code.

arnamoy10 added inline comments.Apr 5 2021, 10:33 AM
flang/lib/Semantics/check-omp-structure.cpp
808

Thanks for pointing in out.

  1. Just double checking -- is parallel region a region enclosed by the !$omp parallel construct?
  2. IIUC, this check (by which my code was inspired) also does not cover the definition of "close nesting"? Please confirm.
flang/lib/Semantics/check-omp-structure.cpp
808
  1. Yes. But it is a dynamic concept, i.e it is the body of code executed during the execution.
  2. Yes that code also does not cover the definition. Will appreciate a fix.
arnamoy10 added inline comments.Apr 5 2021, 2:33 PM
flang/lib/Semantics/check-omp-structure.cpp
808

I need one more clarification before implementing the fix.

From the definition:

A region nested inside another region with no parallel region nested between them

What does nesting "between" mean? I can think of two cases.

Case 1:

!$omp ordered
  !$omp parallel <-- Child of ordered
    !$omp barrier <-- Child of parallel, grandchild of ordered

Case 2:

!$omp ordered
  !$omp parallel <-- Child of ordered
  !$omp barrier <-- Child of ordered

Can you comment on which one of the above two cases has a barrier closely nested inside the ordered construct?

flang/lib/Semantics/check-omp-structure.cpp
808

Case 2 has a barrier closely nested inside ordered.

arnamoy10 added inline comments.Apr 5 2021, 2:44 PM
flang/lib/Semantics/check-omp-structure.cpp
808

Thanks.

So IIUC, basically if your immediate parent (say region1) is not a !$omp parallel, then you are closely nested inside region1.

If that is the case, IMO we do not need a fix. Because GetContextParent() gives us our immediate parent.

flang/lib/Semantics/check-omp-structure.cpp
808

Question is can there be a situation like,

construct 1
   construct 2
        barrier

The current check is only for barrier and construct 2 and you might conclude that there is no error. If construct 2 is not parallel and barrier and construct 1 is not allowed to be nested then you will miss that error, isn't it?

arnamoy10 added inline comments.Apr 5 2021, 2:59 PM
flang/lib/Semantics/check-omp-structure.cpp
808

I see that makes sense.

I think I can restrict the check until the grandparent in the construct stack? For example, I do not need to flag the following case:

not allowed construct 1
  allowed construct 2
    allowed construct 3
       barrier

Or do I?

flang/lib/Semantics/check-omp-structure.cpp
808

Why do you think so?

You might be right for real world benchmarks, since many constructs don't allow other constructs to nest. But theoretically you should.

arnamoy10 updated this revision to Diff 336479.Apr 9 2021, 8:42 AM

Updating patch to use newly landed API for checking close nesting.

Also added a few more test cases when there is violation for more than 1 level of nesting.

kiranchandramohan requested changes to this revision.Apr 10 2021, 6:50 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.h
94

Barriers are allowed in a parallel region. It is probably not allowed in parallel do, do simd etc.

flang/test/Semantics/omp-nested-barrier.f90
45

This is wrong.

111

This and further tests below that use distribute are going to run into the following restriction when it is implemented.

"The region associated with the distribute construct must be strictly nested inside a teams region"

This revision now requires changes to proceed.Apr 10 2021, 6:50 AM
arnamoy10 updated this revision to Diff 350589.Jun 8 2021, 5:57 AM
arnamoy10 marked 3 inline comments as done.

Using the recently introduced OmpDirectiveSet workShareSet to do the nesting check.

flang/lib/Semantics/check-omp-structure.h
94

Thanks, addressed.

flang/test/Semantics/omp-nested-barrier.f90
45

Thanks, updated

111

Thanks, they are currently shown.

This revision is now accepted and ready to land.Jun 10 2021, 12:21 AM
This revision was landed with ongoing or failed builds.Jun 18 2021, 1:23 PM
This revision was automatically updated to reflect the committed changes.