Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP Reduction Clause
ClosedPublic

Authored by yhegde on Nov 3 2020, 10:52 AM.

Details

Summary

Semantic check for OpenMP 4.5 - 2.15.3.6 Reduction clause

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

Test cases:
omp-reduction01.f90
omp-reduction02.f90
omp-reduction03.f90
omp-reduction04.f90
omp-reduction05.f90
omp-reduction06.f90
omp-reduction07.f90
omp-reduction08.f90

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Jan 15 2021, 12:46 PM
yhegde marked 5 inline comments as done.Feb 1 2021, 7:00 AM
yhegde added inline comments.
flang/lib/Semantics/check-directive-structure.h
220

removed multimap which was used only to reuse some part of the code.

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

Few more test cases added with worksharing constructs in omp-reduction07.f90. Hope they are ok.

1520

No such assumptions made. Included few test cases in omp-reduction07.f90 having enclosing context other than parallel construct. Hope they are ok.

1528

I have cleaned up this part of the code. Hope this address the review comment.

1530

Rt now separate error messages are thrown for each of these enclosing clauses . For ex: "REDUCTION variable 'k' is FIRSTPRIVATE in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the worksharing construct bind." or
"REDUCTION variable 'k' is PRIVATE in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the worksharing construct bind." Please let me know if they are not ok.

yhegde updated this revision to Diff 320456.Feb 1 2021, 7:07 AM

This patch has the following changes.

  1. The functions CheckIntentInPointer and CheckDefinableObjects are combined and renamed as 'CheckIntentInPointerAndDefinable'.
  2. Removed CheckDependArraySection function.
  3. cleaned up unwanted code around multimap which was required earlier to reuse some part of the code.
  4. Added few test cases in omp-reduction02.f90 and omp-reduction07.f90 to address the review comments.
  5. Removed some unnecessary code in CheckIsVarPartOfAnotherVar
DavidTruby resigned from this revision.Feb 8 2021, 6:00 AM
kiranchandramohan requested changes to this revision.Feb 10 2021, 4:02 PM

A few questions and change requests.

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

Is name->source and direct comparison with smallcase sufficient?

821

Can this be const?

823

Please add a TODO here.
TODO: Verify the assumption here that the immediately inclosing region is the parallel region to which the worksharing construct having reduction binds to.

Also add a check that the enclosingContext is a parallel construct.

849

Ideally a new symbol should be created for private clauses and reductions (see below). If new symbols are created then how will there be a match in this find? Can you explain?

static constexpr Symbol::Flags ompFlagsRequireNewSymbol{
    Symbol::Flag::OmpPrivate, Symbol::Flag::OmpLinear,
    Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
    Symbol::Flag::OmpReduction};
1516

What I meant here is that if the reduction clause does not appear in a worksharing construct (example below) then the following error does not apply.

!ERROR: REDUCTION variable 'k' is PRIVATE in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the worksharing construct bind.
program omp_reduction
  
  integer :: i,j,l
  integer :: k = 10

  !$omp parallel private(k)
  !$omp simd reduction(+:k)
  do i = 1, 10
    k = k + 1
  end do
  !$omp end simd
  !$omp end parallel

end program omp_reduction
flang/lib/Semantics/resolve-directives.cpp
376

Nit: namepair -> namePair

396

Where was the symbol created for these names?

flang/test/Semantics/omp-reduction07.f90
37

From the standard: "distribute, distribute simd, distribute parallel loop, distribute parallel loop SIMD, and parallel regions, including any parallel regions arising from combined constructs, are the only OpenMP regions that may be strictly nested inside the teams region"

Nesting of an omp do worksharing loop inside a teams region is not allowed. This particular test in this file can be removed.

This revision now requires changes to proceed.Feb 10 2021, 4:02 PM
yhegde marked 2 inline comments as done.Feb 13 2021, 10:01 PM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
756

I think I was converting them to upper case because I came across a few examples with 'max' , 'min' in uppercase and few in lower case.

821

Thank you for commenting on this. I rolled back from the most of common code sharing part.

823

Please add a TODO here.
TODO: Verify the assumption here that the immediately inclosing region is the parallel region to which the worksharing construct having reduction binds to.

Can this be verified with the following cases ?
program red
!$omp sections private(k)

!$omp do reduction(+:k) reduction(-:j)
do i = 1, 10
  k = k + 1
end do
!$omp end do

!$omp end sections
end program red

program red1
!$omp parallel sections private(k)

!$omp do reduction(+:k) reduction(-:j)
do i = 1, 10
  k = k + 1
end do
!$omp end do

!$omp end parallel sections
end program red1

Also add a check that the enclosingContext is a parallel construct.

can the following a valid testcase and hence can be added ?
program red
!$omp target private(k)

!$omp sections  reduction(+:k) reduction(-:j)
do i = 1, 10
  k = k + 1
end do
!$omp end sections

!$omp end target
end program red

849

Thats right. It was overlooked while trying to share the common code. Thank you.

1516

Thank you for explaining. I removed that case from omp-reduction07.f90. I suppose the following can be added?

  1. program red

!$omp sections private(k)

!$omp sections  reduction(+:k) reduction(-:j)
do i = 1, 10
  k = k + 1
end do
!$omp end sections

!$omp end sections
end program red

  1. program red

!$omp parallel sections private(k)

!$omp sections  reduction(+:k) reduction(-:j)
do i = 1, 10
  k = k + 1
end do
!$omp end sections

!$omp end parallel sections
end program red

flang/lib/Semantics/resolve-directives.cpp
396

Not sure I understood this question. I think here symbols are created with IntrinsicOperators.

flang/test/Semantics/omp-reduction07.f90
37

removed.

yhegde updated this revision to Diff 323599.Feb 14 2021, 1:02 AM
yhegde marked an inline comment as done.

Thanks for the review comments. @kiranchandramohan . The patch is updated after addressing the review comments.

A few nits, comments and clarifications.

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

Just confirm, add a test with max and MAX.

823

The ones which you specify for verifying the assumption (program red and program red1) should be caught by the nesting check (i.e. a worksharing region cannot occur in another worksharing region i.e do in sections).

program red
!$omp sections private(k)
!$omp do reduction(+:k) reduction(-:j)
program red1
!$omp parallel sections private(k)
!$omp do reduction(+:k) reduction(-:j)

The one which you specify for the enclosing context is probably a valid case and the reduction error should not apply.

program red
!$omp target private(k)
!$omp sections  reduction(+:k) reduction(-:j)
870

Nit: remove empty line.

876

Nit: remove empty line.

1296

Nit: contiguos -> contiguous

1516

Sections cannot be nested inside another sections since it is a worksharing construct.

flang/lib/Semantics/resolve-directives.cpp
396

I saw symbols created for the reduction operation above but not for the variable.

yhegde marked 3 inline comments as done.Feb 23 2021, 3:10 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
756

The following example is from https://www.openmp.org//wp-content/uploads/openmp-examples-4.5.0.pdf

SUBROUTINE REDUCTION1(A, B, C, D, X, Y, N)
REAL :: X(*), A, D
INTEGER :: Y(*), N, B, C
INTEGER :: I
A = 0
B = 0
C = Y(1)
D = X(1)
!$OMP PARALLEL DO PRIVATE(I) SHARED(X, Y, N) REDUCTION(+:A) &
!$OMP& REDUCTION(IEOR:B) REDUCTION(MIN:C) REDUCTION(MAX:D)
DO I=1,N
A = A + X(I)
B = IEOR(B, Y(I))
C = MIN(C, Y(I))
IF (D < X(I)) D = X(I)
END DO
END SUBROUTINE REDUCTION1

But probably can be ignored ?! and only lower cases sufficient ?

823

The ones which you specify for verifying the assumption (program red and program red1) should be caught by the nesting check (i.e. a worksharing region cannot occur in another worksharing region i.e do in sections).

program red
!$omp sections private(k)
!$omp do reduction(+:k) reduction(-:j)

yes this test case catching both the errors .

./red.f90:3:7: error: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region

!$omp do reduction(+:k) reduction(-:j)
      ^^

./red.f90:3:10: error: REDUCTION variable 'k' is PRIVATE in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the worksharing construct bind.

!$omp do reduction(+:k) reduction(-:j)
         ^^^^^^^^^^^^^^
flang/lib/Semantics/resolve-directives.cpp
396

The symbols were not created for ProcedureDesignator. So they were created here. And I think all other symbols associated with reduction are created along with IntrinsicOperators .

yhegde updated this revision to Diff 325727.Feb 23 2021, 3:24 AM

Patch updated and rebased after addressing the reviewer's comments.

Can you mark all comments which you believe are handled as done?

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

Feel free to add tests. If it is from the standards document then add a comment mentioning the source.

I am thinking that everything is converted to lowercase, but I could be wrong, hence asking to check with both small and caps letters for max.

823

OK.

flang/lib/Semantics/resolve-directives.cpp
396

OK. Is there a test_symbols.sh test which checks for the symbol for non-Intrinsic reduction?

yhegde marked 14 inline comments as done and an inline comment as not done.Feb 24 2021, 12:12 PM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
756

test_symbols.sh throws error for the test cases with MAX,MIN etc in upper case.

1516

ok.

flang/lib/Semantics/resolve-directives.cpp
396

I suppose , you mean the following test cases ?

!$omp do reduction(-:k) reduction(*:j) reduction(-:l)

!DEF: /omp_reduction/Block7/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
do i=1,10
  !REF: /omp_reduction/k
  k = k+1
end do
!$omp end do


!$omp do reduction(.and.:k) reduction(.or.:j) reduction(.eqv.:l)
!DEF: /omp_reduction/Block8/i (OmpPrivate, OmpPreDetermined) HostAssoc INTEGER(4)
do i=1,10
  !REF: /omp_reduction/k
  k = k+1
end do
!$omp end do

I can add them.

yhegde marked an inline comment as done.Feb 27 2021, 11:13 PM
yhegde added inline comments.
flang/lib/Semantics/resolve-directives.cpp
396

Or if the cases similar to the following case are, those under consideration ,

program red
!$omp sections reduction(&:k) reduction(#:j)
do i = 1, 10

k = k + 1

end do
!$omp end sections
end program red

then the test_symbols.sh is catching them.

There are a few comments not marked as done. Are these not done or have been just not marked done?

flang/lib/Semantics/resolve-directives.cpp
396

For the example with loop induction variable "i" and reduction variable "k", the example you show above only contains the OmpPrivate attribute for the loop induction variable "i", it does not show anything for the reduction variable "k". Shouldn't it show OmpReduction for the reduction variables? And shouldn't it be checked?

yhegde marked 5 inline comments as done.Mar 3 2021, 8:53 AM

There are a few comments not marked as done. Are these not done or have been just not marked done?

I believe I have addressed all the issues raised. Hence marking them as done except question type remarks.

flang/lib/Semantics/resolve-directives.cpp
396

Thank you @kiranchandramohan for the review comments. Sorry for the delayed response. Thanks for pointing out the above issue. Calling ResolveOmpObjectList() will resolve this issue. However the recent commits giving some error . Working on that. Will update the patch soon.

yhegde updated this revision to Diff 328384.Mar 4 2021, 10:36 PM

This patch is updated after addressing the review comments.

However there is an error in the build.
The following sections from the omp-firstprivate01.f90 is the source.

!$omp parallel reduction(+:a)
!$omp sections firstprivate(a, b)
!$omp section 
 c = c * a + b
!$omp end sections
!$omp end parallel


!$omp parallel reduction(-:a)
!$omp task firstprivate(a,b)
 c =  c - a * b
!$omp end task
!$omp end parallel

With the above test case
The "enclosingContext{GetEnclosingDirContext()" is returning reduction clause causing the error as -
"REDUCTION variable 'a' is REDUCTION in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the "worksharing" construct bind."

This is because as it appears to me, that the each directive context is pushed including end sections
( void OmpStructureChecker::Enter(const parser::OmpEndSectionsDirective &x) {

case llvm::omp::Directive::OMPD_sections:
PushContextAndClauseSets(
    dir.source, llvm::omp::Directive::OMPD_end_sections);
break;

)
but there is no corresponding pop for the end sections . This is probably because by the std. it is "There is an implicit barrier at the end of a sections construct unless a nowait clause is specified "
( https://www.openmp.org/wp-content/uploads/openmp-4.5.pdf :page 66 )

And it looks to me that "no wait" is not yet implemented and that could be the cause of error for all the worksharing constructs, because the implementation of nowait may have a corresponding pop (dirContext_.pop_back();).

I request the reviewer's suggestions to handle this case.

If popping the end_sections helps you move forward, I am fine with it. Please go ahead.

Does this mean that currently the stack of clause sets is not empty when semantic checks finishes? And with your fix the set will be empty? If so, can you add such an assert (size of clase stack is 0) in the appropriate place?

yhegde updated this revision to Diff 328757.Mar 6 2021, 5:26 AM

Thanks for the suggestion. This patch is rebased and updated with the popping of end_section and other end worksharing clauses fix.

kiranchandramohan accepted this revision.Mar 7 2021, 4:01 PM

LGTM. Thanks for the patience.

@clementval Do you have further concerns about this patch?

flang/lib/Semantics/check-directive-structure.h
209

Nit: Praveen added a "GetEnclosingContextWithDir" with D91920.

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

Nit: Please add a comment on why this pop is required and why it is only for these two directives. If more are to be added later on then please add a todo.

flang/test/Semantics/omp-reduction07.f90
5

Nit: convert one of the tests in this file to work with max/min/....

yhegde added a comment.Mar 8 2021, 5:43 AM

LGTM. Thanks for the patience.

Thank you for the review comments. Sure will update the diff with the suggestions.

clementval accepted this revision.Mar 9 2021, 11:12 AM
This revision is now accepted and ready to land.Mar 9 2021, 11:12 AM
yhegde marked 3 inline comments as done.Mar 10 2021, 10:09 AM
flang/lib/Semantics/check-directive-structure.h
209

They are two different functions. GetEnclosingDirContext() checks the immediate enclosing directive context.

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

There are many begin and end directive mismatch checks in omp-clause-validity01.f90. Hence popping the right directive context was required.

flang/test/Semantics/omp-reduction07.f90
5

omp-reduction07.f90 updated with max/min etc

yhegde updated this revision to Diff 329720.Mar 10 2021, 11:49 AM
yhegde marked 3 inline comments as done.

This patch is updated with a few more test cases with max/min etc in omp-reduction07.f90 and also TODO comments. Hope they are acceptable.

Thanks to @clementval and @kiranchandramohan for accepting this PR and for the review comments. Thanks to @praveen for the kind co operation.

This patch is updated with a few more test cases with max/min etc in omp-reduction07.f90 and also TODO comments. Hope they are acceptable.

Thanks to @clementval and @kiranchandramohan for accepting this PR and for the review comments. Thanks to @praveen for the kind co operation.

Please go ahead and submit.
Thanks for your help and patience @yhegde, @praveen. Goes without saying, You are both welcome to make contributions to Flang and OpenMP in particular.
Best wishes for your future endeavours.

This patch is updated with a few more test cases with max/min etc in omp-reduction07.f90 and also TODO comments. Hope they are acceptable.

Thanks to @clementval and @kiranchandramohan for accepting this PR and for the review comments. Thanks to @praveen for the kind co operation.

Please go ahead and submit.
Thanks for your help and patience @yhegde, @praveen. Goes without saying, You are both welcome to make contributions to Flang and OpenMP in particular.
Best wishes for your future endeavours.

Sure. We look ahead for that. Thank you.