Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs for single directive
ClosedPublic

Authored by yhegde on Dec 14 2020, 4:24 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.7.1 Do Loop restrictions on single directive and firstprivate clause.

Files:
check-directive-structure.h
check-omp-structure.h
check-omp-structure.cpp

Testcases:
omp-do01.f90
omp-do05.f90

Diff Detail

Event Timeline

yhegde created this revision.Dec 14 2020, 4:24 AM
yhegde requested review of this revision.Dec 14 2020, 4:24 AM
clementval added inline comments.Dec 18 2020, 7:42 PM
flang/lib/Semantics/check-directive-structure.h
145

Maybe add a blank line here to keep with the style of this file?

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

Doesn't really check smth but set a state in the context. Should it be renamed ?

106

Remove blank line.

152

dirContext_ act as a stack so do you really intent to go from the bottom of the stack to the top? If so no problem. Just asking.

515

Extra blank line.

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

Remove blank line. Style is not really consistent across file but let's try to keep with the same style in a single file.

yhegde added inline comments.Dec 18 2020, 9:36 PM
flang/lib/Semantics/check-omp-structure.cpp
96

Ok . I can change it to SetLoopInfo .

152

Yes. Check on nesting is called only if Do directive present in that stack.

yhegde added inline comments.Dec 21 2020, 9:42 AM
flang/lib/Semantics/check-omp-structure.h
211

That blank line is left to separate the functions specific to a patch just like a blank line is left before CheckDependList.

yhegde updated this revision to Diff 313135.Dec 21 2020, 9:55 AM

This patch is updated with corrections to positive test cases and indentations.

Why are you changing the tests while removing XFAIL? eg. omp-do05.f90 was supposed to check chunk_size.

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

We capture data-sharing attributes in resolve-directives right? Why is this function better here compared to resolve-directives?

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

Should this call to HasInvalidWorksharingNesting be similar to the one in OpenMPLoopDirective and happen before pushing the context and not iterate over the entire stack of contexts?

Why are you changing the tests while removing XFAIL? eg. omp-do05.f90 was supposed to check chunk_size.

The chunk_size is checked and committed. (https://reviews.llvm.org/D89546) . We found this as a duplicate test case committed earlier by us with XFAIL so thought of re using it. Same with omp-do01.f90 also . Collapse and Ordered with loop levels is already checked and committed. (https://reviews.llvm.org/D89860) Hence we thought of reusing it.

yhegde added inline comments.Dec 21 2020, 10:43 PM
flang/lib/Semantics/check-omp-structure.cpp
152

In OpenMPLoopDirective OMPD_Do context is pushed after calling HasInvalidWorksharingNesting. So I could not do this check in a similar way.

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

Is there any issue with pushing the context after calling HasInvalidWorksharingNesting here also?

yhegde added inline comments.Dec 24 2020, 1:27 AM
flang/lib/Semantics/check-omp-structure.cpp
152

I suppose you mean pushing OMPD_Do context after calling HasInvalidWorksharingNesting ? . If so , then it is not required to push that context later. Control reaches OpenMPLoopDirective first and then OMPD_single.

yhegde added inline comments.Dec 24 2020, 10:58 AM
flang/lib/Semantics/check-omp-structure.cpp
513

Do you mean CheckMultipleAppearances in resolve-directives? I think It works for same or different clauses containing same symbols when they are in the same directive context. or I may be missing something.

yhegde updated this revision to Diff 315362.Jan 8 2021, 5:57 AM

Rebased with some code changes.

yhegde updated this revision to Diff 315588.Jan 9 2021, 3:16 AM

Rebased again.

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

I was asking whether it is OK to implement it the following way so that we don't have to use FindInEnclosingDirContext function?

  if (beginDir.v == llvm::omp::Directive::OMPD_single) {
    HasInvalidWorksharingNesting(...);
  }

  PushContextAndClauseSets(beginDir.source, beginDir.v);

  switch (beginDir.v) {
  case llvm::omp::OMPD_parallel:
    CheckNoBranching(block, llvm::omp::OMPD_parallel, beginDir.source);
    break;
  default:
    break;
  }
}
520

Does the context have the current directive? If so can we get the loopIV through the directive and avoid the need to store the loopIV field?

yhegde added inline comments.Jan 11 2021, 11:53 AM
flang/lib/Semantics/check-omp-structure.cpp
152

FindEnclosingDirContext function can check any restrictive directives for "single" , like "critical" or "task" - if in case it is required to check . Otherwise I can call

if (beginDir.v == llvm::omp::Directive::OMPD_single) {

  HasInvalidWorksharingNesting(HasInvalidWorksharingNesting(
        beginDir.source, {llvm::omp::Directive::OMPD_do}););
}

before pushing the PushContextAndClauseSets(beginDir.source, beginDir.v) and get the expected error.

520

I suppose I require OpenMPLoopConstruct and DoConstruct to get loopIndex. dirContext_ gives directivesource and llvm::omp::Directive , directiveEnum.

Looks mostly OK. Just one issue (FindEnclosingDirContext) to take a decision.

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

If you want to check for restrictions besides OMPD_do, then you can add them to the set like the following isn't it?

if (beginDir.v == llvm::omp::Directive::OMPD_single) {
  HasInvalidWorksharingNesting(
       beginDir.source{llvm::omp::Directive::OMPD_do,
                                   llvm::omp::Directive::OMPD_critical,
                                   llvm::omp::Directive::OMPD_task});
}

Anyway the FindEnclosingDirContext is probably not needed for this patch. If we need it later then we can introduce it there. If you agree please remove FindEnclosingDirContext.

yhegde updated this revision to Diff 315994.Jan 11 2021, 10:24 PM

Addressed the review comments.

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

ok. Then I suppose 'single' binds to immediate outer directive context irrespective of that context. I have moved code before pushing the context. Thanks for the review comments.

yhegde marked 2 inline comments as done.Jan 11 2021, 10:47 PM
flang/lib/Semantics/check-omp-structure.cpp
152

Can you point me to this restriction for single in the standard?

yhegde added inline comments.Jan 12 2021, 6:20 AM
flang/lib/Semantics/check-omp-structure.cpp
152

I was under an impression that FindInEnclosingDircontext check is required in one such following case:

program omp_do

integer a(10),b(10),c(10)

!$omp parallel sections
!$omp section
!$omp critical
!$omp do

do i=1,10

!$omp parallel

a(i) = b(i) + c(i)

!$omp single

j = j + 1

!$omp end single
!$omp end parallel
end do
!$omp end critical
!$omp end parallel sections

But may be it is enough to check immediate outer dir context. So I have updated the diff with suggested changes.

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

To confirm whether it is a closely nested (construct/region) check or a general nested check, I would like to see which specific restriction you are implementing here from the standard. I could not find the single restriction in the worksharing loop section (2.7.1). Did I miss?

However, I see the following in Section 2.17
• A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region

Please clarify which specific restriction you are implementing here.

yhegde added inline comments.Jan 12 2021, 9:32 AM
flang/lib/Semantics/check-omp-structure.cpp
152

The case related to 'single' - (omp-do05.f90) is listed under 2.7.1 with restriction description "All loops associated with the loop construct must be perfectly nested; that is, there must be no intervening code nor any OpenMP directive between any two loops.".

The spec https://www.openmp.org/wp-content/uploads/openmp-examples-4.5.0.pdf , page 277 , gives a similar example calling it as "non-conforming example because the loop and single regions are closely nested.

gfortran throws an error - "work-sharing region may not be closely nested inside of work-sharing, ‘critical’, ‘ordered’, ‘master’, explicit ‘task’ or ‘taskloop’ region" - for these cases.

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

I believe the 2.7.1 restriction "All loops associated with the loop construct must be perfectly nested" refers to cases where there are more than one loop associated with the "omp do", for e.g. when there is a collapse clause.

The example you refer to in page 277 (openmp examples) is under the "restrictions on nesting of regions" section.

And the gfortran error is similar to the nesting restrictions in Section 2.17 (openmp 4.5 standard). So I believe what you are implementing here is a case of the following restriction with the first worksharing region being "single" and the second being "worksharing loop (do)".

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

The standard defines closely nested region as follows in Page 6.

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

So the following example from page 277 you mention should result in an error.

SUBROUTINE PROG(N)
  INTEGER N
  INTEGER I
  !$OMP PARALLEL DEFAULT(SHARED)
  !$OMP DO
  DO I = 1, N
    !$OMP SINGLE ! incorrect nesting of regions
    CALL WORK(I, 1)
    !$OMP END SINGLE
  END DO
  !$OMP END DO
  !$OMP END PARALLEL
END SUBROUTINE PROG

But the following modified version with parallel inserted before single should not result in an error.

SUBROUTINE PROG(N)
  INTEGER N
  INTEGER I
  !$OMP PARALLEL DEFAULT(SHARED)
  !$OMP DO
  DO I = 1, N
    !$OMP PARALLEL
    !$OMP SINGLE ! correct nesting of regions
    CALL WORK(I, 1)
    !$OMP END SINGLE
    !$OMP END PARALLEL
  END DO
  !$OMP END DO
  !$OMP END PARALLEL
END SUBROUTINE PROG

Ideally this could have been implemented as a separate patch for nesting of regions checks. But I do not mind if you continue here.

yhegde added inline comments.Jan 12 2021, 11:44 PM
flang/lib/Semantics/check-omp-structure.cpp
152

Thank you @kiranchandramohan . Shall I update the patch with these two test cases. ?

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

Yes, please add these two tests and also add a TODO in the source code saying that this check will need to be extended or generalised while implementing nesting of regions checks.

Also. update the table entry in nesting of regions for "A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region." from implemented to partially implemented.

yhegde updated this revision to Diff 316351.Jan 13 2021, 2:30 AM

Updated the patch with few more test cases.

yhegde added inline comments.Jan 13 2021, 2:31 AM
flang/lib/Semantics/check-omp-structure.cpp
152

ok.

yhegde updated this revision to Diff 316362.Jan 13 2021, 3:53 AM

Rebased with the latest changes.

kiranchandramohan requested changes to this revision.Jan 13 2021, 5:41 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
520

Is loopIV cleared somewhere? I believe stale values can cause this check to incorrectly fire.

This revision now requires changes to proceed.Jan 13 2021, 5:41 AM
yhegde marked an inline comment as done.Jan 13 2021, 6:49 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
520

ok. Resetting the loopIV in the ResetPartialContext function.

yhegde updated this revision to Diff 316381.Jan 13 2021, 6:58 AM
yhegde marked an inline comment as done.

Addressed the review comments related to loopIV.

LGTM. Thanks for the patience.

This revision is now accepted and ready to land.Jan 13 2021, 7:03 AM