Page MenuHomePhabricator

[Flang][OpenMP 4.5] Add semantic check for OpenMP Reduction Clause
Needs ReviewPublic

Authored by yhegde on Tue, Nov 3, 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

yhegde created this revision.Tue, Nov 3, 10:52 AM
yhegde requested review of this revision.Tue, Nov 3, 10:52 AM

Couple of comments and questions.

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

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.Thu, Nov 12, 5:51 AM

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

This revision now requires changes to proceed.Thu, Nov 12, 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.Fri, Nov 13, 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.Fri, Nov 13, 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.Fri, Nov 13, 1:31 PM
clementval requested changes to this revision.Fri, Nov 13, 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.Tue, Nov 17, 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
450

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.Tue, Nov 24, 11:11 PM
flang/lib/Semantics/check-omp-structure.cpp
450

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
450

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.Wed, Nov 25, 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.