[flang][openmp]At most one threads, simd and depend clause can appear on OpenMP ORDERED construct.

[sameeranjoshi]

Summary

Section 2.17.9 from omp 5.0 has a few restrictions related to ORDERED construct.

• At most one threads clause can appear on an ordered construct.
• At most one simd clause can appear on an ordered construct.
• At most one depend(source) clause can appear on an ordered construct.
• Either depend(sink:vec) clauses or depend(source) clauses may appear on an ordered construct, but not both.

This patch implements them by changing the TableGen file(OMP.td).
The dependencies Simd and Threads clauses were missing semantic checks and were added.

[kiranchandramohan]

Two quick questions.

flang/test/Semantics/omp-clause-validity01.f90

489

Will this catch two or more depend(sink: v) clauses accidentally?

At most one depend(source) clause can appear on an ordered construct.

Can threads, simd occur with depend? Is that handled by the parser?

[clementval]

flang/test/Semantics/omp-clause-validity01.f90

489

According to the standard multiple depend(sink: vec) are allowed so you will need to make a specific check for the depend clause since the general allowedOnce check does not support this.

@kiranchandramohan With the current patch threads, simd and depend are allowed together. The parser is not limiting clauses.

llvm/include/llvm/Frontend/OpenMP/OMP.td

461

OMPC_Depend should probably be in allowedClauses and specific check done in check-omp-structure for the difference between depend(source) and depend(sink:vec)

[sameeranjoshi]

Summary of changes:

• Fix core dump in ordered construct due to missing tree node visit

for OpenMPSimpleStandaloneConstruct as ordered with a depend falls under it.

• Fix internal error in name resolution of depend clause.

Internal error for Critical construct seems to get fixed with this code change.
Not really sure how does that fix, but this will be revisited in D93051.

This diff doesn't yet work on the review comments so as to avoid confusions.

[kiranchandramohan]

Requesting changes as per review comments/questions before. Thanks @clementval.

[sameeranjoshi]

Summary of changes:

• Handle depend(source) and depend(sink:vec) separately.
• Handle one more restriction from standard[5.0][2.17.9] which was needed with this patch. Either depend(sink:vec) clauses or depend(source) clauses may appear on an ordered construct, but not both.

[clementval]

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

782

Why PC? You have Fortran::parser::OmpDependClause pointers directly.

[sameeranjoshi]

Tweak comments as suggested.

[sameeranjoshi]

flang/test/Semantics/omp-clause-validity01.f90

489

Can threads, simd occur with depend? Is that handled by the parser?

They are not supposed to occur together.
No it's not caught in parser.
This will be handled separately. I am seeing some issues need to debug it.

[sameeranjoshi]

Ideally in the first look the checks would fit better for ordered construct
inside a single function CheckDependOnOrderedDir so we have them at a
single place , unfortunately it's harder to implement it in above function
considering that the information to differentiate between a
OmpBlockDirective and OmpSimpleStandaloneDirective is unavailable.

Hence the approach of checks inside the respective nodes was choosen.
Using FindClause doesn't work for some reason hence reverting to the old
school way of finding manually inside a list of OmpClause.

[kiranchandramohan]

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

138

Why is this handled on Leave and not Enter like others?

220

Same question here.

795–832

struct OmpDependenceType {
  ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
};

struct OmpDependClause {
....
  struct InOut {
    TUPLE_CLASS_BOILERPLATE(InOut);
    std::tuple<OmpDependenceType, std::list<Designator>> t;
  };
  std::variant<Source, Sink, InOut> u;
};

Can you use OmpDependenceType in OmpDependClause to simplify this logic? I think you can just count the source, sink and print out the error messages.

[clementval]

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

138

Good point.

[sameeranjoshi]

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

138

With the current approach (traversing an OmpClauseList) it could have worked inside Enter.
But here's are a few reasons not to use

• I prefer doing checks inside Leave, so that the checks remain at a single location and Enter does the context pushing and other stuff.
• I had initially plan to use FindClause as that saves us from writing more code and code looks more readable, unfortunately that did not work in the first run as ResetPartialContext clears the members in DirectiveContext which has a reason because OmpBeginBlockDirective & OmpEndBlockDirective have different set of clauses based on the context.

To resolve this will add Leave for OmpBeginBlockDirective.

void Enter(const parser::OpenMPBlockConstruct &);
void Leave(const parser::OpenMPBlockConstruct &);
void Leave(const parser::OmpBeginBlockDirective &);
void Enter(const parser::OmpEndBlockDirective &);

• FindClause doesn't work inside Enter as the list is empty and it's populated only when we visit each clause. grep FindClause and see it's not found inside any Enter function.

220

The case of OpenMPSimpleStandaloneConstruct is different as there is no ResetPartialContext nor any end constructs nor directives for it.
So doing the check here has below reason:

• FindClause will only work inside Leave.
• A style approach.

With the next diff I have plan to use FindClause.

[sameeranjoshi]

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

795–832

Could anyone please elaborate more on above?
I couldn't understand .
parse-tree.h mentions

// 2.13.9 depend-type -> IN | OUT | INOUT | SOURCE | SINK
struct OmpDependenceType {
  ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
  WRAPPER_CLASS_BOILERPLATE(OmpDependenceType, Type);
};

Where as parsers omit's the source and sink in OmpDependenceType in openmp-parsers.cpp

TYPE_PARSER(
    construct<OmpDependenceType>("IN"_id >> pure(OmpDependenceType::Type::In) ||
        "INOUT" >> pure(OmpDependenceType::Type::Inout) ||
        "OUT" >> pure(OmpDependenceType::Type::Out)))

I was confused if the parse tree has some issues.
Hence, if you somecan elaborate a bit on what exactly is expected that would be really helpful.
Or if there's a more better way of simplifying this code.

[clementval]

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

138

What is blocking you now to use FindClause? The ResetPartialContext should not prevent you to use FindClause since you are checking different sets of clauses.

[sameeranjoshi]

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

138

I will send a patch for the same.
I am trying to understand one more comment
https://reviews.llvm.org/D94087#inline-889166

[kiranchandramohan]

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

138

Isn't FindClause used when you are inside a clause and hence do not have access to the construct? If you do it in Enter or Leave of the construct you have access to all the clauses. You might be able to use parser::Unwrap and check whether a clause is present.

795–832

I was assuming that the dependence_type is set always. If it is not then can it be set in the parser and used? Is there any reason why it is not set?

[clementval]

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

138

FindClause might be faster since it's a simple query on a map already populated when the clause is check if it is allowed.

[sameeranjoshi]

Summary of changes:

• Refactor and simplify the algorithm without changes in parser.
• Added 2 routines for code cleanup, and help extended for the later checks. ChecksOnOrderedAsStandalone ChecksOnOrderedAsBlock
• Use FindClause to replace manual traversal of OmpClauseList. FindClause didn't work inside Leave(const parser::OpenMPBlockConstruct &) as the DirectiveContext gets reset before we reach the node. Hence checks are now done in Leave(const parser::OmpBeginBlockDirective &)

[sameeranjoshi]

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

795–832

I have updated to simplify the logic. I am trying to not change parser in first run as there might be some reason why that might have been designed by original authors.
I have no strong opinions to change the parser if this logic still seems harder.
Thanks for parser::Unwrap inputs that helped to simplify things @kiranchandramohan.

[clementval]

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

793

Clauses should be capitalized in message.

[sameeranjoshi]

Capitalized clauses in message.

[clementval]

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

781

What is the final reason to not use FindClause? It would be more efficient than iterate over the clause list once more.

[kiranchandramohan]

Will need a rebase now.

[peixin]

BTW, the error of no symbol found in depend(sink: vec) should be fixed by https://reviews.llvm.org/D108530.

flang/test/Semantics/omp-clause-validity01.f90

489

We may should also give error infomation when threads, simd occur with depend. Otherwise, the behavior would be unspecified. For current clang codegen, it generates the IR only for depend when threads, simd occur with depend. I am implementing the code for flang codegen. I should also use the similar method as clang codegen. The method should be like this:

if (isDependPresent) {
  // lowering depend
} else if (isSimdPresent) {
  // lowering simd
} else {
  // It does not matter if isThreadsPresent is true or false since the construct behaves as if threads is specified when no clause is specified.
}

I assume that this can be checked in Leave(const parser::OmpClauseList &) by using FindClause. The condition should be like this:

if ((isThreadsPresent || isSimdPresent) && isDependPresent) {
  // error report
}

Maybe there is one better place to check this.

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

#include "llvm/Frontend/OpenMP/OMP.inc"
   void Enter(const parser::OmpClause::NumThreads &);
   void Enter(const parser::OmpClause::Ordered &);
   void Enter(const parser::OmpClause::Priority &);
   void Enter(const parser::OmpClause::Private &);
   void Enter(const parser::OmpClause::ProcBind &);
   void Enter(const parser::OmpClause::Reduction &);
   void Enter(const parser::OmpClause::Safelen &);
   void Enter(const parser::OmpClause::Shared &);
+  void Enter(const parser::OmpClause::Simd &);
   void Enter(const parser::OmpClause::Simdlen &);
   void Enter(const parser::OmpClause::TaskReduction &);
+  void Enter(const parser::OmpClause::Threads &);
   void Enter(const parser::OmpClause::ThreadLimit &);
   void Enter(const parser::OmpClause::To &);
   void Enter(const parser::OmpClause::Link &);
   void Enter(const parser::OmpClause::Uniform &);
   void Enter(const parser::OmpClause::UseDevicePtr &);
   void Enter(const parser::OmpClause::IsDevicePtr &);
   // Memory-order-clause
   void Enter(const parser::OmpClause::SeqCst &);

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

void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
   case llvm::omp::OMPD_parallel:
     CheckNoBranching(block, llvm::omp::OMPD_parallel, beginDir.source);
     break;
   default:
     break;
   }
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) {
   const auto &beginSectionsDir{
       std::get<parser::OmpBeginSectionsDirective>(x.t)};
   const auto &endSectionsDir{std::get<parser::OmpEndSectionsDirective>(x.t)};
   const auto &beginDir{

 
 void OmpStructureChecker::Enter(
     const parser::OpenMPSimpleStandaloneConstruct &x) {
   const auto &dir{std::get<parser::OmpSimpleStandaloneDirective>(x.t)};
   PushContextAndClauseSets(dir.source, dir.v);
 }
 
 void OmpStructureChecker::Leave(
     const parser::OpenMPSimpleStandaloneConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPFlushConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_flush);
 }
 

 CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel)
 CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire)
 CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
 CHECK_SIMPLE_CLAUSE(Release, OMPC_release)
 CHECK_SIMPLE_CLAUSE(Relaxed, OMPC_relaxed)
 CHECK_SIMPLE_CLAUSE(Hint, OMPC_hint)
 CHECK_SIMPLE_CLAUSE(ProcBind, OMPC_proc_bind)
 CHECK_SIMPLE_CLAUSE(DistSchedule, OMPC_dist_schedule)
+CHECK_SIMPLE_CLAUSE(Simd, OMPC_simd)
+CHECK_SIMPLE_CLAUSE(Threads, OMPC_threads)
 
 CHECK_REQ_SCALAR_INT_CLAUSE(Allocator, OMPC_allocator)
 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)

 }
 
 void OmpStructureChecker::CheckIntentInPointer(
     const parser::OmpObjectList &objectList, const llvm::omp::Clause clause) {
   std::vector<const Symbol *> symbols;
   GetSymbolsInObjectList(objectList, symbols);
   for (const auto *symbol : symbols) {
     if (IsPointer(*symbol) && IsIntentIn(*symbol)) {
       context_.Say(GetContext().clauseSource,
           "Pointer '%s' with the INTENT(IN) attribute may not appear "
           "in a %s clause"_err_en_US,
           symbol->name(),
           parser::ToUpperCaseLetters(getClauseName(clause).str()));
     }
   }
 }
 
 void OmpStructureChecker::GetSymbolsInObjectList(
     const parser::OmpObjectList &objectList,
     std::vector<const Symbol *> &symbols) {
   for (const auto &ompObject : objectList.v) {
     if (const auto *name{parser::Unwrap<parser::Name>(ompObject)}) {
       if (const auto *symbol{name->symbol}) {
         if (const auto *commonBlockDetails{
                 symbol->detailsIf<CommonBlockDetails>()}) {
           for (const auto &object : commonBlockDetails->objects()) {
             symbols.emplace_back(&object->GetUltimate());
           }
         } else {
           symbols.emplace_back(&symbol->GetUltimate());
         }
       }
     }
   }
 }
 
 } // namespace Fortran::semantics

flang/test/Semantics/omp-clause-validity01.f90

! Standalone Directives (basic)
   !ERROR: Internal: no symbol found for 'i'
   !$omp taskwait depend(sink:i-1)
   ! !$omp target enter data map(to:arrayA) map(alloc:arrayB)
   ! !$omp target update from(arrayA) to(arrayB)
   ! !$omp target exit data map(from:arrayA) map(delete:arrayB)
   !$omp ordered depend(source)
   !ERROR: Internal: no symbol found for 'i'
   !$omp ordered depend(sink:i-1)
+  !ERROR: At most one DEPEND clause can appear on the ORDERED directive
+  !$omp ordered depend(source) depend(source)
+  !ERROR: At most one THREADS clause can appear on the ORDERED directive
+  !$omp ordered threads threads
+  !$omp end ordered
+  !ERROR: At most one SIMD clause can appear on the ORDERED directive
+  !$omp ordered simd simd
+  !$omp end ordered
   !$omp flush (c)
   !$omp flush acq_rel
   !$omp flush release
   !$omp flush acquire
   !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
   !$omp flush release (c)
   !ERROR: SEQ_CST clause is not allowed on the FLUSH directive
   !$omp flush seq_cst

llvm/include/llvm/Frontend/OpenMP/OMP.td

let allowedOnceClauses = [
     VersionedClause<OMPC_Acquire, 50>,
     VersionedClause<OMPC_Release, 50>,
     // TODO This should ne `none` instead. Comment carried over from
     // OMPKinds.def.
     VersionedClause<OMPC_Flush>
   ];
 }
 def OMP_Ordered : Directive<"ordered"> {
-  let allowedClauses = [
+  let allowedOnceClauses = [
     VersionedClause<OMPC_Threads>,
     VersionedClause<OMPC_Simd>,
     VersionedClause<OMPC_Depend>
   ];
 }
 def OMP_Atomic : Directive<"atomic"> {
   let allowedClauses = [
     VersionedClause<OMPC_Read>,
     VersionedClause<OMPC_Write>,
     VersionedClause<OMPC_Update>,
     VersionedClause<OMPC_Capture>,