[PATCH] D91920: [Flang] [OpenMP] Add semantic checks for OpenMP firstprivate , lastprivate and copyprivate clauses

[Praveen G via Phabricator via llvm-commits]

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


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1015
+        std::visit(common::visitors{
+                       [&](const parser::OmpClause::Private &x) {
+                         if (enclosingClauseSet.test(
----------------
kiranchandramohan wrote:
> Having to hard-code this looks unfortunate. Can you add a TODO to replace this hardcoding? Try one of the following,
> 1) autogen checks
> 2) or an autogenerated function which maps parser::OmpClause::<name> to llvm::omp::Clause::OMPC_<name>
@kiranchandramohan Will try to add autogenerated function / autogen checks for the same


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1033-1040
+                       [&](const parser::OmpClause::Reduction &x) {
+                         if (enclosingClauseSet.test(
+                                 llvm::omp::Clause::OMPC_reduction)) {
+                           const auto &ompObjectList{
+                               std::get<parser::OmpObjectList>(x.v.t)};
+                           GetSymbolsInObjectList(
+                               ompObjectList, enclosingSymbols);
----------------
kiranchandramohan wrote:
> Will this clash with the reduction work that @yhegde is doing in https://reviews.llvm.org/D90697?
@kiranchandramohan In this patch , only the variables in the firstprivate and lastprivate clauses on the worksharing constructs are being checked while in https://reviews.llvm.org/D90697 added by @yhegde , variables in the reduction clause is checked . So , I dont think there would be a clash regarding the checks.

But the checks for privates and reduction in the enclosing region is common for both.  


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:864
+  if (currDirSet.test(GetContext().directive)) {
+    if (auto *enclosingContext{GetEnclosingDirContext()}) {
+      if (enclosingDirSet.test(enclosingContext->directive)) {
----------------
kiranchandramohan wrote:
> praveen wrote:
> > kiranchandramohan wrote:
> > > praveen wrote:
> > > > praveen wrote:
> > > > > kiranchandramohan wrote:
> > > > > > Is there an assumption here that the enclosing directive is the parallel/teams region?
> > > > > > 
> > > > > > Can it happen that the enclosing immediate directive is not the parallel/teams region to which the worksharing construct binds? Something like the following? 
> > > > > > ```
> > > > > > parallel private(x)
> > > > > >   some_omp_construct
> > > > > >     worksharing_construct firstprivate(x)
> > > > > > ```
> > > > > @kiranchandramohan I am not sure if there can be any immediate enclosing directives for workshare constructs . 
> > > > > 
> > > > > Since we already check 'HasInvalidWorksharingNesting' , do we need to check for the '//firstprivate/lastprivate variables that are private in enclosing context//' for the variables on the worksharing constructs?
> > > > > 
> > > > > The task construct can contain directives other than parallel such as the following.
> > > > > 
> > > > >   !$omp parallel reduction (+:a)
> > > > >     !$omp single
> > > > >       !$omp task firstprivate (a)
> > > > >         a = a + b
> > > > >       !$omp end task
> > > > >     !$omp end single
> > > > >   !$omp end parallel
> > > > > 
> > > > > Should we thrown an error for the cases such as the one above ?
> > > > @kiranchandramohan Have updated the code to identify the enclosing constructs that are not immediate enclosing directives and binds to the current directive.
> > > ```
> > > A list item that appears in a reduction clause of a parallel construct must not appear in a
> > > firstprivate clause on a worksharing, task, or taskloop construct if any of the
> > > worksharing or task regions arising from the worksharing, task, or taskloop construct ever
> > > bind to any of the parallel regions arising from the parallel construct.
> > > ```
> > > 
> > > As per the above statement from the standard, yes it has to be flagged. If there are issues in handling this then please let me know.
> > @kiranchandramohan I have updated the code to handle cases such as the above example where the binding region may  not be the immediate enclosing directive.
> Could you point me to where and how this was handled?
@kiranchandramohan The function **GetEnclosingContextWithDir** returns the matching directive in the dirContext and it is called in check-omp-structure.cpp:1011

  DirectiveContext *GetEnclosingContextWithDir(D dir) {
    CHECK(!dirContext_.empty());
    auto it{dirContext_.rbegin()};
    while (++it != dirContext_.rend()) {
      if (it->directive == dir) {
        return &(*it);
      }
    }
    return nullptr;
  }


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

https://reviews.llvm.org/D91920