[PATCH] D90697: [Flang][OpenMP 4.5] Add semantic check for OpenMP Reduction Clause

[Yashaswini Hegde via Phabricator via llvm-commits]

yhegde marked 14 inline comments as done and an inline comment as not done.
yhegde added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:970
+
+void OmpStructureChecker::CheckCommonSymbolsInOuterCxt(
+    SymbolSourceMap &currSymbols, const llvm::omp::Clause currClause,
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > yhegde wrote:
> > > > kiranchandramohan wrote:
> > > > > 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.
> > > > Few more test cases added with worksharing constructs in omp-reduction07.f90.  Hope they are ok. 
> > > 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
> > > ```
> > 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
> > 
> > 2.  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
> > 
> > 
> Sections cannot be nested inside another sections since it is a worksharing construct.
ok.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:542
+              std::string procName =
+                  parser::ToUpperCaseLetters(name->symbol->name().ToString());
+              if (procName == "MAX" || procName == "MIN" ||
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > yhegde wrote:
> > > > kiranchandramohan wrote:
> > > > > Is name->source and direct comparison with smallcase sufficient?
> > > > 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. 
> > > Just confirm, add a test with max and MAX.
> > 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 ?
> 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.
test_symbols.sh throws error for the test cases with MAX,MIN etc in upper case. 


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:349
+        CheckMultipleAppearances(
+            *name, *name->symbol, Symbol::Flag::OmpReduction);
+      }
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > yhegde wrote:
> > > > kiranchandramohan wrote:
> > > > > Where was the symbol created for these names?
> > > > Not sure I understood this question.  I think here symbols are created with IntrinsicOperators. 
> > > I saw symbols created for the reduction operation above but not for the variable.
> > 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 .
> OK. Is there a test_symbols.sh test which checks for the symbol for non-Intrinsic reduction?
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90697/new/

https://reviews.llvm.org/D90697