[PATCH] D93205: [Flang][OpenMP 4.5] Add semantic check for OpenMP Do Loop Constructs for single directive
Kiran Chandramohan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 13 01:18:29 PST 2021
kiranchandramohan added inline comments.
================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:153
+ case llvm::omp::OMPD_single:
+ for (const auto &dirContext : dirContext_) {
+ if (dirContext.directive == llvm::omp::Directive::OMPD_do) {
----------------
yhegde wrote:
> kiranchandramohan wrote:
> > yhegde wrote:
> > > kiranchandramohan wrote:
> > > > yhegde wrote:
> > > > > kiranchandramohan wrote:
> > > > > > yhegde wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > yhegde wrote:
> > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > yhegde wrote:
> > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > yhegde wrote:
> > > > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > > > yhegde wrote:
> > > > > > > > > > > > > > > clementval wrote:
> > > > > > > > > > > > > > > > `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.
> > > > > > > > > > > > > > > Yes. Check on nesting is called only if Do directive present in that stack.
> > > > > > > > > > > > > > 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?
> > > > > > > > > > > > > In OpenMPLoopDirective OMPD_Do context is pushed after calling HasInvalidWorksharingNesting. So I could not do this check in a similar way.
> > > > > > > > > > > > Is there any issue with pushing the context after calling HasInvalidWorksharingNesting here also?
> > > > > > > > > > > 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.
> > > > > > > > > > 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;
> > > > > > > > > > }
> > > > > > > > > > }
> > > > > > > > > > ```
> > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > 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.
> > > > > > > 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.
> > > > > > Can you point me to this restriction for single in the standard?
> > > > > 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.
> > > > 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.
> > > 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.
> > 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.
> Thank you @kiranchandramohan . Shall I update the patch with these two test cases. ?
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93205/new/
https://reviews.llvm.org/D93205
More information about the llvm-commits
mailing list