[PATCH] D93091: [Flang] [OpenMP] Add semantic checks for OpenMP Workshare Construct

[Praveen G via Phabricator via llvm-commits]

praveen marked 2 inline comments as done.
praveen added a comment.

In D93091#2495365 <https://reviews.llvm.org/D93091#2495365>, @kiranchandramohan wrote:

> Add a few tests to show that most restrictions do not apply inside a parallel construct nested inside a workshare construct.

@kiranchandramohan Added a class to make use of parse-tree walk for identifying all the assignments and expressions in workshare construct . Added tests with parallel constructs inside a workshare construct.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:867
+      } else {
+
+        // Check if OpenMP constructs enclosed in the Workshare Construct are
----------------
kiranchandramohan wrote:
> Nit: Remove empty line.
Done


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:912-914
+          "The structured block in a WORKSHARE construct may consist of only "
+          "scalar or array assignments, forall or where statements, "
+          "forall, where, atomic, critical or parallel constructs"_err_en_US);
----------------
kiranchandramohan wrote:
> Nit: Capitalize constructs like elsewhere.
Done


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:766
+              context_.Say(expr.source,
+                  "User defined non-ELEMENTAL function "
+                  "'%s' is not allowed in a WORKSHARE construct"_err_en_US,
----------------
praveen wrote:
> kiranchandramohan wrote:
> > praveen wrote:
> > > praveen wrote:
> > > > kiranchandramohan wrote:
> > > > > I think use of non-elemental function can be outside an assignment statement like in the where statement below.
> > > > > ```
> > > > > program mn
> > > > >   integer :: a(10) = (/1,2,3,4,5,6,7,8,9,10/)
> > > > >   !$omp workshare
> > > > >   where ( a .lt. f()) a = a + 5
> > > > >   !$omp end workshare
> > > > > contains
> > > > >   integer function f()
> > > > >     f = 5
> > > > >   end function
> > > > > end program
> > > > > ```
> > > > Thanks @kiranchandramohan Will handle the same.
> > > @kiranchandramohan Modified the code to handle the same . Thanks!
> > There are probably more places (see example below) where user-defined are not allowed. I wonder whether a parse-tree walk would have been better for catching issues like these.
> > ```
> > program mn
> > integer, parameter ::N = 100
> > real :: A(N,N), B(N,N)
> > !$omp workshare
> > FORALL(I = f():N, J = f():N, A(I, J) .NE. f()) B(I, J) = 1.0 / A(I, J)
> > !$omp end workshare
> > contains
> >  pure integer function f()
> >     f = 5
> >   end function
> > end program
> > ```
> @kiranchandramohan Thanks for pointing this out . I had missed this check in forall statements and construct. 
> 
> I do feel parse-tree walk would be better to catch all the assignments and Expressions instead of multiple functions for each statement / construct.
Have added a new class to use parse-tree walk to catch all the assignments and Expressions.


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

https://reviews.llvm.org/D93091