[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 ↗

(On Diff #299669)

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 ↗

(On Diff #299669)

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 ↗

(On Diff #299669)

Which bot do you see failing?

[sameeranjoshi]

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

423 ↗

(On Diff #299669)

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 ↗

(On Diff #299669)

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

551

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

flang/test/Semantics/omp-flush.f90

36 ↗

(On Diff #302333)

Any reason to have upper and lower cases?

Same questions for the others below

flang/test/Semantics/omp-flush01.f90

17

Any reason to have upper and lower cases?

Same questions for the others below

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

423 ↗

(On Diff #302333)

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

218

Isn't this line exceeding the character limit?

flang/test/Semantics/omp-flush01.f90

28

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

28

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

28

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

[sameeranjoshi]

flang/test/Semantics/omp-flush01.f90

28

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

28

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

28

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.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 (FindClause(llvm::omp::Clause::OMPC_acquire) ||
+      FindClause(llvm::omp::Clause::OMPC_release) ||
+      FindClause(llvm::omp::Clause::OMPC_acq_rel)) {
+    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);
+    }
+  }
   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);
 }
 
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)};
     const auto &modType2{
         std::get<std::optional<parser::OmpScheduleModifier::Modifier2>>(
             modifier->t)};
     if (modType1.v.v == type || (modType2 && modType2->v.v == type)) {

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)
   !ERROR: SEQ_CST clause is not allowed on the FLUSH directive
   !$omp flush seq_cst
   !ERROR: RELAXED clause is not allowed on the FLUSH directive
   !$omp flush relaxed
 
   !$omp cancel DO
   !$omp cancellation point parallel

flang/test/Semantics/omp-flush01.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+
+! 2.17.8 Flush construct [OpenMP 5.0]
+!        memory-order-clause ->
+!                               acq_rel
+!                               release
+!                               acquire
+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
+
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush private(array)
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush num_threads(4)
+
+    ! Mix allowed and not allowed clauses.
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush seq_cst acquire
+    !ERROR: expected end of line
+    !ERROR: expected end of line
+    !$omp flush num_threads(4) acquire
+  end if
+  !$omp end parallel
+end
+

flang/test/Semantics/omp-flush02.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+
+! Check OpenMP 5.0 - 2.17.8 flush Construct
+! Restriction -
+! If memory-order-clause is release, acquire, or acq_rel, list items must not be specified on the flush directive.
+
+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/)
+  !Only memory-order-clauses.
+  if (omp_get_thread_num() == 1) THEN
+    ! Not allowed clauses.
+    !ERROR: SEQ_CST clause is not allowed on the FLUSH directive
+    !$omp flush seq_cst
+
+    !ERROR: RELAXED clause is not allowed on the FLUSH directive
+    !$omp flush relaxed
+  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
+