[Flang][OpenMP-5.0] Semantic checks for flush construct.

[sameeranjoshi]

Summary

From OMP 5.0 [2.17.8]
Restriction:
If memory-order-clause is release,acquire, or acq_rel, list items must not be specified on the flush directive.

[sameeranjoshi]

For a test as below

  TYPE somestr
    real :: rr
  end type
  TYPE(somestr) :: structObj

!$omp flush (structObj%rr)

I could see nag, intel and gfortran producing error when a list item is structObj%rr.
I didn't find inside standard though.
So should this be flagged as well an error?

[sameeranjoshi]

Rebased on top of master to try to resolve buildbot failure.
https://reviews.llvm.org/harbormaster/build/99902/

[clementval]

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

423

Did you check if clang test are still passing with this change?

[sameeranjoshi]

I usually don't build clang.
I need to try.

[sameeranjoshi]

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

423

With ninja check-clang check-flang all seems well.
I can't figure out why bots failed in applying the patch.
@Reviewers can someone conform if this patch applies and build and tests well?

[clementval]

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

423

Which bot do you see failing?

[sameeranjoshi]

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

423

From[1] I specifically failing[2] to apply patch.

[1] https://reviews.llvm.org/harbormaster/build/99908/
[2] https://buildkite.com/llvm-project/diff-checks/builds/16004

[clementval]

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

423

Looks like it a conflict problem. You should rebase one more time.

[sameeranjoshi]

Rebase on master.

[sameeranjoshi]

This commit tries to get a workaround for not chaning the parser as well as adding semantic checks.
The function FindClause was used earlier to get information from context.
Above function works on clauses which come under parser::OmpClause, currently flush doesn't fall under it.

[clementval]

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

601

It makes little sense to use the OmpClause infrastructure to check something enforced by the parser.

flang/test/Semantics/omp-flush.f90

36

Any reason to have upper and lower cases?

Same questions for the others below

flang/test/Semantics/omp-flush01.f90

16

Any reason to have upper and lower cases?

Same questions for the others below

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

423

Since you are not using the OmpClause this change is not needed here.

[sameeranjoshi]

Based on top of the parser changes which were handled in D91839.
Rebased after D91839.
Address review comments.

[clementval]

Some small comments

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

184

Isn't this line exceeding the character limit?

flang/test/Semantics/omp-flush01.f90

29

Why is the error reported twice here? Would it not be nicer to have a list of memory-order-clause and then the error being reported by the semantic check? I mean it would be more user friendly.

[sameeranjoshi]

Work on review comments.

[sameeranjoshi]

flang/test/Semantics/omp-flush01.f90

29

It's a known issue due the improper error recovery in openmp.
omp-atomic.f90 file too had to get a workaround using the same way.

Yes, I have forwarded the errors to semantic phase.

[clementval]

flang/test/Semantics/omp-flush01.f90

29

If you have forwarded the errors to semantic then why is there still two syntax error lines?

[sameeranjoshi]

flang/test/Semantics/omp-flush01.f90

29

The errors forwarded to semantics are something like
At most one XYZ clause can appear on the FLUSH directive.
Generally the ones which don't have clauses apart from memory-order-clause.

The test cases over here have clauses which don't fall under memory-order-clause, hence the parser becomes very stricter over here and catches them in parsing stage.

[clementval]

LGTM. Please wait for the approval from someone from the OpenMP side.

flang/test/Semantics/omp-flush01.f90

29

Ok I see. It would be nice to fix the double reporting of error here but I guess it can be done in a separate patch. Can you add an entry in the progress document to not forget this issue.

[sameeranjoshi]

Thank you!

flang/test/Semantics/omp-flush01.f90

29

Kiran Kumar from AMD was looking into improving the error messages for OMP parts.

[sameeranjoshi]

@kiranchandramohan can you please look into this?

[kiranchandramohan]

LGTM.

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

#include "llvm/Frontend/OpenMP/OMP.cpp.inc"
   void Enter(const parser::OmpDependClause &);
   void Enter(const parser::OmpDistScheduleClause &);
   void Enter(const parser::OmpIfClause &);
   void Enter(const parser::OmpLinearClause &);
   void Enter(const parser::OmpMapClause &);
   void Enter(const parser::OmpProcBindClause &);
   void Enter(const parser::OmpReductionClause &);
   void Enter(const parser::OmpScheduleClause &);
+  void Enter(const parser::OmpFlushMemoryClause &);
 
 private:
 
   bool HasInvalidWorksharingNesting(
       const parser::CharBlock &, const OmpDirectiveSet &);
 
   // specific clause related
   bool ScheduleModifierHasType(const parser::OmpScheduleClause &,
       const parser::OmpScheduleModifierType::ModType &);
 
   llvm::StringRef getClauseName(llvm::omp::Clause clause) override;
   llvm::StringRef getDirectiveName(llvm::omp::Directive directive) override;
 };
 } // namespace Fortran::semantics
 #endif // FORTRAN_SEMANTICS_CHECK_OMP_STRUCTURE_H_

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

void OmpStructureChecker::Leave(
   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);
 }
 
-void OmpStructureChecker::Leave(const parser::OpenMPFlushConstruct &) {
+void OmpStructureChecker::Leave(const parser::OpenMPFlushConstruct &x) {
+  if (const auto &flushMemClause{
+          std::get<std::optional<parser::OmpFlushMemoryClause>>(x.t)}) {
+    switch (flushMemClause.value().v) {
+    case llvm::omp::Clause::OMPC_release:
+    case llvm::omp::Clause::OMPC_acq_rel:
+    case llvm::omp::Clause::OMPC_acquire:
+      if (const auto &flushList{
+              std::get<std::optional<parser::OmpObjectList>>(x.t)}) {
+        context_.Say(parser::FindSourceLocation(flushList),
+            "If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive"_err_en_US);
+      }
+      break;
+    default:
+      break;
+    }
+  }
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPCancelConstruct &x) {
   const auto &dir{std::get<parser::Verbatim>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_cancel);
 }
 

 }
 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);
 }
 
+void OmpStructureChecker::Enter(const parser::OmpFlushMemoryClause &x) {
+  switch (x.v) {
+  case llvm::omp::Clause::OMPC_acquire:
+    CheckAllowed(llvm::omp::Clause::OMPC_acquire);
+    break;
+  case llvm::omp::Clause::OMPC_release:
+    CheckAllowed(llvm::omp::Clause::OMPC_release);
+    break;
+  case llvm::omp::Clause::OMPC_acq_rel:
+    CheckAllowed(llvm::omp::Clause::OMPC_acq_rel);
+    break;
+  default:
+    break;
+  }
+}
+
 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{
         std::get<parser::OmpScheduleModifier::Modifier1>(modifier->t)};

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

! Standalone Directives (basic)
   ! !$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)
   !$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)
   !$omp cancel DO
   !$omp cancellation point parallel
 
 ! 2.13.2 critical Construct
 
   !ERROR: Internal: no symbol found for 'first'
   !$omp critical (first)

flang/test/Semantics/omp-flush.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! Check OpenMP 5.0 - 2.17.8 flush Construct
+
+  use omp_lib
+  implicit none
+
+  TYPE someStruct
+    REAL :: rr
+  end TYPE
+  integer :: i, a, b
+  real, DIMENSION(10) :: array
+  TYPE(someStruct) :: structObj
+
+  a = 1.0
+  !$omp parallel num_threads(4)
+  !No list flushes all.
+  if (omp_get_thread_num() == 1) THEN
+    !$omp flush
+  END IF
+
+  array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+  ! No memory-order-clause only list-items.
+  if (omp_get_thread_num() == 2) THEN
+    !$omp flush (a)
+    !$omp flush (i, a, b)
+    !$omp flush (array, structObj%rr)
+    ! Too many flush with repeating list items.
+    !$omp flush (i, a, b, a, a, a, a, a, a, a, a, a, a, a, a, a, a, a, b, b, b, b)
+    !ERROR: No explicit type declared for 'notpresentitem'
+    !$omp flush (notPresentItem)
+  END IF
+
+  array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+  if (omp_get_thread_num() == 3) THEN
+    !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
+    !$omp flush ACQ_REL (array)
+    !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
+    !$omp flush ACQ_REL (array, a, i)
+
+    array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+    !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
+    !$omp flush RELEASE (array)
+    !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
+    !$omp flush RELEASE (array, a)
+
+    array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+    !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
+    !$omp flush ACQUIRE (array)
+    !ERROR: If memory-order-clause is RELEASE, ACQUIRE, or ACQ_REL, list items must not be specified on the FLUSH directive
+    !$omp flush ACQUIRE (array, a, structObj%rr)
+  END IF
+  !$omp end parallel
+
+  !$omp parallel num_threads(4)
+    array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+    !$omp master
+      !$omp flush (array)
+    !$omp end master
+  !$omp end parallel
+end
+

flang/test/Semantics/omp-flush01.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+! Check OpenMP 5.0 - 2.17.8 flush Construct
+! These checks are caught in parsing as the parser is more strict.
+
+  use omp_lib
+  implicit none
+
+  integer :: i, a, b
+  real, DIMENSION(10) :: array
+
+  a = 1.0
+  !$omp parallel num_threads(4)
+  !Only memory-order-clauses.
+  if (omp_get_thread_num() == 1) THEN
+    ! Allowed clauses.
+    !$omp flush ACQ_REL
+    array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+    !$omp flush RELEASE
+    array = (/1, 2, 3, 4, 5, 6, 7, 8, 9, 10/)
+    !$omp flush ACQUIRE
+
+    ! Not allowed more than once.
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush ACQ_REL ACQ_REL
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush RELEASE RELEASE
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush ACQUIRE ACQUIRE
+
+    ! Not allowed clauses.
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush SEQ_CST
+
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush RELAXED
+
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush PRIVATE(array)
+
+    ! Mix allowed and not allowed clauses.
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush SEQ_CST ACQUIRE
+  END IF
+  !$omp end parallel
+end

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

 }
 def OMP_TaskGroup : Directive<"taskgroup"> {
   let allowedClauses = [
     VersionedClause<OMPC_TaskReduction>,
     VersionedClause<OMPC_Allocate>
   ];
 }
 def OMP_Flush : Directive<"flush"> {
-  let allowedClauses = [
+  let allowedOnceClauses = [
     VersionedClause<OMPC_AcqRel, 50>,
     VersionedClause<OMPC_Acquire, 50>,
     VersionedClause<OMPC_Release, 50>,
-    // TODO This should ne `none` instead. Comment carried over from
+    // TODO This should be `none` instead. Comment carried over from
     // OMPKinds.def.
     VersionedClause<OMPC_Flush>
   ];
 }
 def OMP_Ordered : Directive<"ordered"> {
   let allowedClauses = [
     VersionedClause<OMPC_Threads>,
     VersionedClause<OMPC_Simd>,