Page MenuHomePhabricator

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

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
503

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?

510–523

Nit: Remove commented code.

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

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
503
  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
503

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
503

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
503

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
503

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
503

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
503

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.Sat, Apr 10, 6:50 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.h
82

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
44

This is wrong.

110

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.Sat, Apr 10, 6:50 AM