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.



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

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?


Nit: Remove commented code.

arnamoy10 added inline comments.Apr 5 2021, 10:33 AM

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

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?


Case 2 has a barrier closely nested inside ordered.

arnamoy10 added inline comments.Apr 5 2021, 2:44 PM


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.


Question is can there be a situation like,

construct 1
   construct 2

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

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

Or do I?


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.

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


This is wrong.


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