This is an archive of the discontinued LLVM Phabricator instance.

[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
yhegde requested review of this revision.Nov 3 2020, 10:52 AM

Couple of comments and questions.

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

The check and the error message seem very OpenMP specific. It would be better to move this to the OpenMP specific file. If it is reusable by OpenACC the error message should be passed to the function.

flang/test/Semantics/omp-reduction01.f90
12

Indent does not follows other tests already upstreamed.

flang/test/Semantics/omp-reduction02.f90
13

Same comment for the indentation.

flang/test/Semantics/omp-reduction03.f90
14

Indentation

flang/test/Semantics/omp-reduction04.f90
12

Indentation

flang/test/Semantics/omp-reduction05.f90
14

Indentation

flang/test/Semantics/omp-reduction06.f90
14

Indentation

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

Indentation

Thanks for this patch.

Can you check what is happening with this test?

      subroutine sb(q,nx)
      integer :: q(100)
      integer :: t1, t8
!$OMP PARALLEL DO PRIVATE(k,t1) REDUCTION(max:t8)
      do k=1,nx
        t1=q(k)
        t8=max(t8,t1)
      enddo
      end
kiranchandramohan requested changes to this revision.Nov 12 2020, 5:51 AM

Would need a response for questions above to proceed to acceptance.

This revision now requires changes to proceed.Nov 12 2020, 5:51 AM

Thanks for this patch.

Can you check what is happening with this test?

      subroutine sb(q,nx)
      integer :: q(100)
      integer :: t1, t8
!$OMP PARALLEL DO PRIVATE(k,t1) REDUCTION(max:t8)
      do k=1,nx
        t1=q(k)
        t8=max(t8,t1)
      enddo
      end

Thank you very much @kiranchandramohan and @clementval for all the review comments. Working on this test case. Looks like there is nothing defined for max, min etc like parser::DefinedOperator::IntrinsicOperator is defined for Add, Subtract etc. I have attempted to define them. Will send the patch soon. Please let me know if the changes are in rt direction. Thanks again.
regards

@clementval had some changes for the OpenACC reduction operator in the following commit. I am not sure whether it is related to this issue.
https://reviews.llvm.org/D86296

@clementval had some changes for the OpenACC reduction operator in the following commit. I am not sure whether it is related to this issue.
https://reviews.llvm.org/D86296

Yeah that's what triggered the patch on the OpenACC side. Not sure you can follow the same path for OpenMP since you have to support user defined reduction operator.

yhegde updated this revision to Diff 305204.Nov 13 2020, 10:17 AM

Thank you both @kiranchandramohan and @clementval for your review comments and inputs.

I have considered MAX, MIN, IAND, IOR and IEOR as IntrinsicProcedures. Hope that is ok. Positive test cases for the same are added.

tskeith requested changes to this revision.Nov 13 2020, 1:31 PM
tskeith added a subscriber: tskeith.
tskeith added inline comments.
flang/include/flang/Parser/parse-tree.h
590 ↗(On Diff #305204)

This change doesn't belong here, it should be in OpenMP-specific code. A defined-operator in Fortran does not include any intrinsic procedures.

flang/lib/Parser/Fortran-parsers.cpp
110 ↗(On Diff #305204)

Same problem here. This accepts interface operator(max) which is not valid Fortran.

This revision now requires changes to proceed.Nov 13 2020, 1:31 PM
clementval requested changes to this revision.Nov 13 2020, 5:19 PM
clementval added inline comments.
flang/include/flang/Parser/parse-tree.h
590 ↗(On Diff #305204)

Thanks @tskeith for looking at this. +1

flang/include/flang/Semantics/symbol.h
403 ↗(On Diff #305204)

Not sure if this is the way to go since you add only a fraction of intrinsic procedure her

yhegde updated this revision to Diff 305856.Nov 17 2020, 11:05 AM
yhegde marked 2 inline comments as done.

Thanks for review comments.
The code changes with this patch includes ResolvingNames for max, min, iand, ior, and ieor , assuming that the reduction clause can take user defined procedures.

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

Is some code missing here?

Is that the reason why the following code does not work?

program test
  integer :: y
  !$omp parallel reduction(max:y)
    y = 1
  !$omp end parallel
end program
yhegde added inline comments.Nov 24 2020, 11:11 PM
flang/lib/Semantics/check-omp-structure.cpp
769

That failure was due to missing Name symbol in ProcedureDesignator. I have tried to fix it. Now please let me know the following is the positive case? or do I need to catch it?
program test

integer :: y
!$omp parallel  reduction(foo:y)
  y = foo(1,0)
!$omp end parallel

end program

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

That failure was due to missing Name symbol in ProcedureDesignator. I have tried to fix it. Now please let me know the following is the positive case? or do I need to catch it?
program test

integer :: y
!$omp parallel  reduction(foo:y)
  y = foo(1,0)
!$omp end parallel

end program

Catch it as in provide an error if there is no declare reduction for foo? If so that can be left for the semantics check of declare reduction. Please add an entry in the excel sheet for this.

yhegde updated this revision to Diff 307755.Nov 25 2020, 10:13 PM

This patch resolves the name symbol for the procedure, called in Reduction clause.
A new test case is added -
omp-reduction10.f90.

Semantic check of declare reduction will be addressed in the next patch.

clementval requested changes to this revision.Dec 16 2020, 7:49 AM
clementval added inline comments.
flang/lib/Semantics/check-directive-structure.h
297

Add a blank line here.

308

Can you add a blank line here? The rest of the file has blank lines between functions.

310

Maybe I miss the point here but the function itself does not check for any private attribute on the symbol or smith like that. It just check whether the symbol is in the list of reductionSymbol which can be populated differently.

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

Should be removed?

731

Comment should be removed?

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

Indent is wrong. See clang-format message below.

flang/test/Semantics/omp-reduction08.f90
7

Should you use test_symbols.sh if you want to check this kind of information?

This revision now requires changes to proceed.Dec 16 2020, 7:49 AM
yhegde marked an inline comment as not done.Dec 19 2020, 3:43 AM
yhegde added inline comments.
flang/lib/Semantics/check-directive-structure.h
310

A list item that appears on reduction clause cannot appear in the private clause. This function is initiated from private clause, checks whether the same symbol appeared on the reduction clauses from the same context.

yhegde updated this revision to Diff 312987.Dec 20 2020, 9:29 AM

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

The following are not implemented. They also don't seem to be in the list of items to do. Do you know why they were not added?
-> The type and the rank of a list item that appears in a reduction clause must be valid for the combiner and initializer.
-> Additionally, the list item or the pointer component of the list item must not be deallocated, allocated, or pointer assigned within the region.
-> Additionally, the list item or the allocatable component of the list item must be neither deallocated nor allocated within the region.

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

Should this be a list? Can it be a vector or llvm::SmallVector?
https://llvm.org/docs/ProgrammersManual.html#list

flang/lib/Semantics/check-omp-structure.cpp
785–787

I believe similar functionality was implemented before. eg CheckIntentInPointer. Can it be reused? If not and if they are simple then consider inlining it here.

845

CheckDependArraySection has a similar check. Can you either reuse or refactor such that common code is not duplicated?

851

This will not catch cases like the following.

program mn
  integer :: c(10,10,10)
  integer :: i
  integer :: k = 10

  !$omp parallel do reduction(+:c(1:10,5,1:6))
  do i = 1, 10
    k = k + 1
  end do
  !$omp end parallel do
  print *, is_contiguous(c(1:10,5,1:6))
end program
854

continuous -> contiguous?

919

This does not seem to work for the following case. I think you should start from the reduction list and the check whether those variables are private.

program omp_reduction

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

The following are not implemented. They also don't seem to be in the list of items to do. Do you know why they were not added?

I think we were not very sure that we can catch them at compile time. But I suppose type and rank of a list item can be checked at compile time.

-> The type and the rank of a list item that appears in a reduction clause must be valid for the combiner and initializer.
-> Additionally, the list item or the pointer component of the list item must not be deallocated, allocated, or pointer assigned within the region.
-> Additionally, the list item or the allocatable component of the list item must be neither deallocated nor allocated within the region

yhegde marked an inline comment as done.Dec 22 2020, 2:29 AM
yhegde added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
845

Sure. Will check what all can be reused.

851

Thank you. This will be addressed.

919

Thank you once again. This case will be addressed.

yhegde updated this revision to Diff 314097.Dec 30 2020, 5:50 AM
yhegde marked 5 inline comments as done.

Addressed review issues. Code with similar checks are reused. There are minor changes in depend clause related checks because same function is called from reduction clause related checks.

yhegde updated this revision to Diff 314655.Jan 5 2021, 10:47 AM

With updated Reduction Clause (OmpObjectList).

yhegde updated this revision to Diff 314675.Jan 5 2021, 11:37 AM

With updated Reduction Clause.

yhegde marked 2 inline comments as done.Jan 5 2021, 11:40 AM
kiranchandramohan requested changes to this revision.Jan 15 2021, 12:46 PM

A few comments.

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

In a multimap there can be more than one clause which maps to the same key (i.e more than one clause of the same type). Should there be an assert for only single existence?

298

Is this function OpenMP specific and hence be in the OpenMPChecker?

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

Nit: Remove enclosingSymbols if not used.

782

I guess this function can also work with ompObjectList like others. If you iterate over the names then you dont need the map from symbol to name source.

1499

I guess this function should only be called if the reduction appears in a worksharing construct. Currently there seems to be no checks for that.

1503

There seems to be an assumption here that the immediate enclosing context is the parallel region. Is that always the case?

1511

Is there an assumption here that for non-sharable clauses a new symbol is created? Is that always the case? If so, can you add a comment here?

1513

Consider making this error more readable.

You can state the restriction in the standard, then add a comma and say that reduction variable x is <private/first/lastprivate> here.

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

Remove this function if not used anymore.

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
219

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

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

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

1503

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

1511

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

1513

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
739

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

804

Can this be const?

806

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.

832

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};
1499

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
739

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.

804

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

806

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

832

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

1499

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
739

Just confirm, add a test with max and MAX.

806

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)
853

Nit: remove empty line.

859

Nit: remove empty line.

1279

Nit: contiguos -> contiguous

1499

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
739

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 ?

806

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
739

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.

806

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
739

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

1499

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
208

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
208

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.