[Flang][OpenMP][NFC][1/2] Reorder OmpStructureChecker and simplify it.

[sameeranjoshi]

Summary

OmpStructureChecker has too much boilerplate code in source file.
It was not easy to figure out the seperation of clauses inside 'OmpClause' and
the ones which had a seperate node in parse-tree.h.

This patch:

1. Removes the boilerplate by defining a few macros.
2. Makes seperation between constructs, directives and clauses(sub classes are seperated).
3. Macros could have been shared between OMP and OACC, template specilizations might have been costly hence used macros.

Follows the same strategy used for AccStructureChecker.

Next patch in series to simplify OmpStructureChecker would try to simplify
boilerplates inside the functions and either create abstractions or use if
something is available inside check-directive-structure.h

[clementval]

We might even go further and generate these with TableGen. I guess we have most of the information in OMP.td or ACC.td for that. Anyway might be wise to first land this patch and look at TableGen in a next step. Would be happy to look at it if it makes sense.

Anyway LGTM. Will let OpenMP folks comment and accept it.

[kiranchandramohan]

We might even go further and generate these with TableGen. I guess we have most of the information in OMP.td or ACC.td for that.
Anyway might be wise to first land this patch and look at TableGen in a next step. Would be happy to look at it if it makes sense.

+1

Makes seperation between constructs, directives and clauses(sub classes are seperated).

Is this change in this patch?

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

21

The name is a bit misleading since it does CheckAllowed also.

373–407

Nit: seperate -> separate. Used in a few other places and in the summary.

[sameeranjoshi]

Makes seperation between constructs, directives and clauses(sub classes are seperated).

Is this change in this patch?

Yes, the file has

1. Constructs and directives at the beginning.
2. Clauses in the later half. The clauses are themselves divided as
   1. First half has clauses which are inside OmpClause.
   2. Second half for clauses which have a special node in parse-tree.h. e.g OmpMemoryOrderClause and similar ones might come here.

Before this patch they were somewhat mixed in the file.

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

21

I see CheckAllowed used at all places, hence that is assumed as that is necessary.
CHECK_REQ_SCALAR_INT_CLAUSE is as well shared with AccStructureChecker.
Do you have a better suggestion here?

[kiranchandramohan]

Another way would be to classify various OpenMP clauses similar to what is done in PFT.
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/include/flang/Lower/PFTBuilder.h#L115
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/include/flang/Lower/PFTBuilder.h#L145

And then use that classification to do what we want for different OpenMP clause classes.
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/lib/Lower/PFTBuilder.cpp#L80

Would that be possible?

[sameeranjoshi]

In D90324#2366643, @kiranchandramohan wrote:

Another way would be to classify various OpenMP clauses similar to what is done in PFT.
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/include/flang/Lower/PFTBuilder.h#L115
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/include/flang/Lower/PFTBuilder.h#L145

And then use that classification to do what we want for different OpenMP clause classes.
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/lib/Lower/PFTBuilder.cpp#L80

Would that be possible?

Thank you for suggestion.

May be it seems possible.
What benefit do you see compared to the current approach?
I see we can group all clauses at one location, compared to the one I mentioned below.

Anticipating the status of OMP/OACC for the next 4 months I think the macros would be replaced with Enter/Leave functions with more checks in them from standard.
for e.g something like Enter(const parser::OmpClause::Ordered &x)
This function has other restrictions implemented from standard specific to the clause apart from tablegen changes.

So the macros are somewhat partial checks, having macros makes us easily figure out in this case whether the restrictions on clause are partially implemented or more detailed.

[kiranchandramohan]

What benefit do you see compared to the current approach?

We cannot put a breakpoint in macros.

[clementval]

So the macros are somewhat partial checks, having macros makes us easily figure out in this case whether the restrictions on clause are partially implemented or more detailed.

I don't fully agree on this one. At least on the OpenACC side lots of clauses will have only those checks and thus keep the macro or TableGen style.

[sameeranjoshi]

In D90324#2366660, @clementval wrote:

So the macros are somewhat partial checks, having macros makes us easily figure out in this case whether the restrictions on clause are partially implemented or more detailed.

I don't fully agree on this one. At least on the OpenACC side lots of clauses will have only those checks and thus keep the macro or TableGen style.

IIUC, TableGen would check only for allowed, notallowed, etc and the restrictions in TableGen.

Where would the other restrictions be checked?
e.g Enter(const parser::OmpScheduleClause &x) there are too many checks.

Are they done somewhere in OACC which I might be unaware of ?

[clementval]

IIUC, TableGen would check only for allowed, notallowed, etc and the restrictions in TableGen.

Where would the other restrictions be checked?
e.g Enter(const parser::OmpScheduleClause &x) there are too many checks.

That's not what I meant. What I meant is that lots of clauses have just trivial checks and therefore will just use the macros and nothing else. In your statement your were mentioning that those were just temporary but in my opinion (at least on the OpenACC side) they will stay as is since they do not have further restrictions.

[sameeranjoshi]

In D90324#2366730, @clementval wrote:

IIUC, TableGen would check only for allowed, notallowed, etc and the restrictions in TableGen.

Where would the other restrictions be checked?
e.g Enter(const parser::OmpScheduleClause &x) there are too many checks.

That's not what I meant. What I meant is that lots of clauses have just trivial checks and therefore will just use the macros and nothing else. In your statement your were mentioning that those were just temporary but in my opinion (at least on the OpenACC side) they will stay as is since they do not have further restrictions.

True for OpenACC, but OMP has restrictions on clauses apart from trivial checks(I haven't checked the standard on which clauses might be exclusion to this statement), but at least what I can see from the patches which @yhegde and @praveen pushed they mostly implement these clauses.

What should be the final decision on this patch finally?

[clementval]

What should be the final decision on this patch finally?

Looks fine for me but wait for someone closer to OpenMP to double approve it.

[kiranchandramohan]

Thanks for the cleanup. If you can remove the macros in a later patch that would be great.

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

 //===-- lib/Semantics/check-omp-structure.cpp -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "check-omp-structure.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/tools.h"
 
 namespace Fortran::semantics {
 
+// Use when clause falls under 'struct OmpClause' in 'parse-tree.h'.
+#define CHECK_SIMPLE_CLAUSE(X, Y) \
+  void OmpStructureChecker::Enter(const parser::OmpClause::X &) { \
+    CheckAllowed(llvm::omp::Clause::Y); \
+  }
+
+#define CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(X, Y) \
+  void OmpStructureChecker::Enter(const parser::OmpClause::X &c) { \
+    CheckAllowed(llvm::omp::Clause::Y); \
+    RequiresConstantPositiveParameter(llvm::omp::Clause::Y, c.v); \
+  }
+
+#define CHECK_REQ_SCALAR_INT_CLAUSE(X, Y) \
+  void OmpStructureChecker::Enter(const parser::OmpClause::X &c) { \
+    CheckAllowed(llvm::omp::Clause::Y); \
+    RequiresPositiveParameter(llvm::omp::Clause::Y, c.v); \
+  }
+
+// Use when clause don't falls under 'struct OmpClause' in 'parse-tree.h'.
+#define CHECK_SIMPLE_PARSER_CLAUSE(X, Y) \
+  void OmpStructureChecker::Enter(const parser::X &) { \
+    CheckAllowed(llvm::omp::Y); \
+  }
+
 bool OmpStructureChecker::HasInvalidWorksharingNesting(
     const parser::CharBlock &source, const OmpDirectiveSet &set) {
   // set contains all the invalid closely nested directives
   // for the given directive (`source` here)
   if (CurrentDirectiveIsNested() && set.test(GetContext().directive)) {
     context_.Say(source,
         "A worksharing region may not be closely nested inside a "
         "worksharing, explicit task, taskloop, critical, ordered, atomic, or "
case llvm::omp::Directive::OMPD_workshare:
     SetContextAllowed(OmpClauseSet{llvm::omp::Clause::OMPC_nowait});
     break;
   default:
     // no clauses are allowed
     break;
   }
 }
 
+// Clauses
+// Mainly categorized as
+// 1. Checks on 'OmpClauseList' from 'parse-tree.h'.
+// 2. Checks on clauses which fall under 'struct OmpClause' from parse-tree.h.
+// 3. Checks on clauses which are not in 'struct OmpClause' from parse-tree.h.
+
 void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
   // 2.7 Loop Construct Restriction
   if (llvm::omp::doSet.test(GetContext().directive)) {
     if (auto *clause{FindClause(llvm::omp::Clause::OMPC_schedule)}) {
       // only one schedule clause is allowed
       const auto &schedClause{std::get<parser::OmpScheduleClause>(clause->u)};
       if (ScheduleModifierHasType(schedClause,
               parser::OmpScheduleModifierType::ModType::Nonmonotonic)) {
void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
   GetContext().requiredClauses.IterateOverMembers(
       [this](llvm::omp::Clause c) { CheckRequired(c); });
 }
 
 void OmpStructureChecker::Enter(const parser::OmpClause &x) {
   SetContextClause(x);
 }
 
-void OmpStructureChecker::Enter(const parser::OmpNowait &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_nowait);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Inbranch &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_inbranch);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Mergeable &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_mergeable);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Nogroup &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_nogroup);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Notinbranch &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_notinbranch);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Untied &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_untied);
-}
-
-void OmpStructureChecker::Enter(const parser::OmpClause::Collapse &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_collapse);
-  // collapse clause must have a parameter
-  RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_collapse, x.v);
-}
-
-void OmpStructureChecker::Enter(const parser::OmpClause::Copyin &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_copyin);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Copyprivate &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_copyprivate);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Device &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_device);
-}
-void OmpStructureChecker::Enter(const parser::OmpDistScheduleClause &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_dist_schedule);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Final &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_final);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Firstprivate &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_firstprivate);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::From &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_from);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Grainsize &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_grainsize);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_grainsize, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Lastprivate &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_lastprivate);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::NumTasks &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_num_tasks);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_num_tasks, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::NumTeams &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_num_teams);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_num_teams, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::NumThreads &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_num_threads);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_num_threads, x.v);
-  // if parameter is variable, defer to Expression Analysis
-}
+// Following clauses do not have a seperate node in parse-tree.h.
+// They fall under 'struct OmpClause' in parse-tree.h.
+CHECK_SIMPLE_CLAUSE(Copyin, OMPC_copyin)
+CHECK_SIMPLE_CLAUSE(Copyprivate, OMPC_copyprivate)
+CHECK_SIMPLE_CLAUSE(Device, OMPC_device)
+CHECK_SIMPLE_CLAUSE(Final, OMPC_final)
+CHECK_SIMPLE_CLAUSE(Firstprivate, OMPC_firstprivate)
+CHECK_SIMPLE_CLAUSE(From, OMPC_from)
+CHECK_SIMPLE_CLAUSE(Inbranch, OMPC_inbranch)
+CHECK_SIMPLE_CLAUSE(IsDevicePtr, OMPC_is_device_ptr)
+CHECK_SIMPLE_CLAUSE(Lastprivate, OMPC_lastprivate)
+CHECK_SIMPLE_CLAUSE(Link, OMPC_link)
+CHECK_SIMPLE_CLAUSE(Mergeable, OMPC_mergeable)
+CHECK_SIMPLE_CLAUSE(Nogroup, OMPC_nogroup)
+CHECK_SIMPLE_CLAUSE(Notinbranch, OMPC_notinbranch)
+CHECK_SIMPLE_CLAUSE(Private, OMPC_private)
+CHECK_SIMPLE_CLAUSE(Shared, OMPC_shared)
+CHECK_SIMPLE_CLAUSE(To, OMPC_to)
+CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
+CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
+CHECK_SIMPLE_CLAUSE(UseDevicePtr, OMPC_use_device_ptr)
+
+CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
+CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
+CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
+CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
+CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
+CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)
+
+CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(Collapse, OMPC_collapse)
+CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(Safelen, OMPC_safelen)
+CHECK_REQ_CONSTANT_SCALAR_INT_CLAUSE(Simdlen, OMPC_simdlen)
 
+// Restrictions specific to each clause are implemented apart from the
+// generalized restrictions.
 void OmpStructureChecker::Enter(const parser::OmpClause::Ordered &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_ordered);
   // the parameter of ordered clause is optional
   if (const auto &expr{x.v}) {
     RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_ordered, *expr);
 
     // 2.8.3 Loop SIMD Construct Restriction
     if (llvm::omp::doSimdSet.test(GetContext().directive)) {
       context_.Say(GetContext().clauseSource,
           "No ORDERED clause with a parameter can be specified "
           "on the %s directive"_err_en_US,
           ContextDirectiveAsFortran());
     }
   }
 }
-void OmpStructureChecker::Enter(const parser::OmpClause::Priority &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_priority);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_priority, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Private &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_private);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Safelen &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_safelen);
-  RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_safelen, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Shared &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_shared);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Simdlen &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_simdlen);
-  RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_simdlen, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::ThreadLimit &x) {
-  CheckAllowed(llvm::omp::Clause::OMPC_thread_limit);
-  RequiresPositiveParameter(llvm::omp::Clause::OMPC_thread_limit, x.v);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::To &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_to);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Link &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_link);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::Uniform &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_uniform);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::UseDevicePtr &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_use_device_ptr);
-}
-void OmpStructureChecker::Enter(const parser::OmpClause::IsDevicePtr &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_is_device_ptr);
-}
 
+// Following clauses have a seperate node in parse-tree.h.
+CHECK_SIMPLE_PARSER_CLAUSE(OmpAllocateClause, OMPC_allocate)
+CHECK_SIMPLE_PARSER_CLAUSE(OmpDefaultClause, OMPC_default)
+CHECK_SIMPLE_PARSER_CLAUSE(OmpDependClause, OMPC_depend)
+CHECK_SIMPLE_PARSER_CLAUSE(OmpDistScheduleClause, OMPC_dist_schedule)
+CHECK_SIMPLE_PARSER_CLAUSE(OmpNowait, OMPC_nowait)
+CHECK_SIMPLE_PARSER_CLAUSE(OmpProcBindClause, OMPC_proc_bind)
+CHECK_SIMPLE_PARSER_CLAUSE(OmpReductionClause, OMPC_reduction)
+
+// Restrictions specific to each clause are implemented apart from the
+// generalized restrictions.
 void OmpStructureChecker::Enter(const parser::OmpAlignedClause &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_aligned);
 
   if (const auto &expr{
           std::get<std::optional<parser::ScalarIntConstantExpr>>(x.t)}) {
     if (const auto v{GetIntValue(*expr)}) {
       if (*v <= 0) {
         context_.Say(GetContext().clauseSource,
             "The ALIGNMENT parameter of the ALIGNED clause must be "
             "a constant positive integer expression"_err_en_US);
       }
     }
   }
   // 2.8.1 TODO: list-item attribute check
 }
-void OmpStructureChecker::Enter(const parser::OmpAllocateClause &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_allocate);
-}
-void OmpStructureChecker::Enter(const parser::OmpDefaultClause &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_default);
-}
 void OmpStructureChecker::Enter(const parser::OmpDefaultmapClause &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_defaultmap);
   using VariableCategory = parser::OmpDefaultmapClause::VariableCategory;
   if (!std::get<std::optional<VariableCategory>>(x.t)) {
     context_.Say(GetContext().clauseSource,
         "The argument TOFROM:SCALAR must be specified on the DEFAULTMAP "
         "clause"_err_en_US);
   }
 }
-void OmpStructureChecker::Enter(const parser::OmpDependClause &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_depend);
-}
-
 void OmpStructureChecker::Enter(const parser::OmpIfClause &x) {
   CheckAllowed(llvm::omp::Clause::OMPC_if);
 
   using dirNameModifier = parser::OmpIfClause::DirectiveNameModifier;
   static std::unordered_map<dirNameModifier, OmpDirectiveSet>
       dirNameModifierMap{{dirNameModifier::Parallel, llvm::omp::parallelSet},
           {dirNameModifier::Target, llvm::omp::targetSet},
           {dirNameModifier::TargetEnterData,
case llvm::omp::Directive::OMPD_target_exit_data: {
             ContextDirectiveAsFortran());
       }
     } break;
     default:
       break;
     }
   }
 }
-void OmpStructureChecker::Enter(const parser::OmpProcBindClause &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_proc_bind);
-}
-void OmpStructureChecker::Enter(const parser::OmpReductionClause &) {
-  CheckAllowed(llvm::omp::Clause::OMPC_reduction);
-}
 
 bool OmpStructureChecker::ScheduleModifierHasType(
     const parser::OmpScheduleClause &x,
     const parser::OmpScheduleModifierType::ModType &type) {
   const auto &modifier{
       std::get<std::optional<parser::OmpScheduleModifier>>(x.t)};
   if (modifier) {
     const auto &modType1{