[flang][OpenMP] Enhance parser support for flush construct to OpenMP 5.0

[kiranktp]

Summary

This patch enhances parser support for flush construct to OpenMP 5.0 by including memory-order-clause.

2.17.8 flush Construct
!$omp flush [memory-order-clause] [(list)]

		where memory-order-clause is
		acq_rel
		release
		acquire

The patch includes code changes and testcase modifications.

[klausler]

LGTM; thanks!

[echristo]

Hi All,

The commit for this ended up failing so I temporarily reverted it in 64b04e4754bfe7bf718e5140fe1fd0ca50373c28. Feel free to recommit on fix :)

Thanks!

-eric

[kiranchandramohan]

Two comments.

memory-order-clause is defined for both atomic and flush instructions. We already have MemoryOrder
defined in the parse-tree for atomic in 4.5. In 4.5 memory-order can only be seq_cst. In 5.0 memory-order
for atomic can be seq_cst, acq_rel, release, acquire, relaxed. For flush it is only a subset and the allowed
values are only acq_rel, release, acquire.

Would it make sense to have a common definition for memory-order-clause and handle the fact that it
can only be a subset in semantics?

Flush is 2.17.8 (not 2.18.8) as part of the OpenMP 5.0 specification here. Is there a newer document?
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

[kiranktp]

In D82177#2105199, @echristo wrote:

Hi All,

The commit for this ended up failing so I temporarily reverted it in 64b04e4754bfe7bf718e5140fe1fd0ca50373c28. Feel free to recommit on fix :)

Thanks!

-eric

Hi Eric,
I had validated the patch with debug build and "ninja check-flang"/"ninja check-all" before pushing it for review on phabricator.
I didn't face any errors. Anyway, I will check with release build once.

[kiranktp]

In D82177#2105223, @kiranchandramohan wrote:

Two comments.

memory-order-clause is defined for both atomic and flush instructions. We already have MemoryOrder
defined in the parse-tree for atomic in 4.5. In 4.5 memory-order can only be seq_cst. In 5.0 memory-order
for atomic can be seq_cst, acq_rel, release, acquire, relaxed. For flush it is only a subset and the allowed
values are only acq_rel, release, acquire.

Would it make sense to have a common definition for memory-order-clause and handle the fact that it
can only be a subset in semantics?

Flush is 2.17.8 (not 2.18.8) as part of the OpenMP 5.0 specification here. Is there a newer document?
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

Thanks for the review Kiran.

memory-order-clause is defined for both atomic and flush instructions. We already have MemoryOrder
defined in the parse-tree for atomic in 4.5. In 4.5 memory-order can only be seq_cst. In 5.0 memory-order
for atomic can be seq_cst, acq_rel, release, acquire, relaxed. For flush it is only a subset and the allowed
values are only acq_rel, release, acquire.

Would it make sense to have a common definition for memory-order-clause and handle the fact that it
can only be a subset in semantics?

I did thought of this approach, but keeping the definitions separate will be neat and will be easier to extend each construct for further changes in future.
I did see the same approach for couple of constructs handling.

Flush is 2.17.8 (not 2.18.8) as part of the OpenMP 5.0 specification here. Is there a newer document?
https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

Thanks for pointing it out. I was referring to OpenMP 5.1 draft version as well. I have used the same section numbers.
I will change it back to OpenMP 5.0 section number.
Probably i will have to change it for other construct as well.

[kiranktp]

Corrected the section number to 2.17.8 [from OpenMP 5.0] for flush construct

[tskeith]

What is happening with this change? It looks like it was applied and then reverted.
@echristo, was there a problem?

[kiranktp]

In D82177#2111512, @tskeith wrote:

What is happening with this change? It looks like it was applied and then reverted.
@echristo, was there a problem?

Hi @echristo,
I am not facing any test failures with this patch on my sandbox. I tested this on multiple setups.
Below are my build details:

env CC=clang-8 CXX=clang++-8 CXXFLAGS="-stdlib=libc++" LDFLAGS="-fuse-ld=lld" cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=../install_clang -DLLVM_TARGETS_TO_BUILD=host -DLLVM_INSTALL_UTILS=on -DLLVM_ENABLE_PROJECTS="mlir;flang" ../../llvm
ninja -j8 install
ninja check-all

Let me know if I am missing anything.

[kiranchandramohan]

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

[echristo]

In D82177#2123184, @kiranchandramohan wrote:

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

I'm happy to try again if you think that the current diff should work :)

-eric

[kiranktp]

In D82177#2123184, @kiranchandramohan wrote:

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

Thank you for Validating Kiran!

[kiranktp]

In D82177#2123312, @echristo wrote:

In D82177#2123184, @kiranchandramohan wrote:

@echristo was seeing a failure when running the test "Semantics/omp-clause-validity01.f90" as per his revert commit message.

Tests pass for me. I built and tested on an ubuntu Aarch64 machine. Flang was built as a release with debug build using gcc-9.2 with ninja.

I'm happy to try again if you think that the current diff should work :)

-eric

Hi Eric, I had shared my build details. Am I missing anything while building/testing?

Please do try and let me know if you still face the issue.

flang/include/flang/Parser/dump-parse-tree.h

#define NODE(T1, T2) NODE_NAME(T1::T2, #T2)
   NODE(OpenMPCancelConstruct, If)
   NODE(parser, OpenMPCancellationPointConstruct)
   NODE(parser, OpenMPConstruct)
   NODE(parser, OpenMPCriticalConstruct)
   NODE(parser, OpenMPDeclarativeConstruct)
   NODE(parser, OpenMPDeclareReductionConstruct)
   NODE(parser, OpenMPDeclareSimdConstruct)
   NODE(parser, OpenMPDeclareTargetConstruct)
+  NODE(parser, OmpFlushMemoryClause)
+  NODE_ENUM(OmpFlushMemoryClause, FlushMemoryOrder)
   NODE(parser, OpenMPFlushConstruct)
   NODE(parser, OpenMPLoopConstruct)
   NODE(parser, OpenMPSimpleStandaloneConstruct)
   NODE(parser, OpenMPStandaloneConstruct)
   NODE(parser, OpenMPSectionsConstruct)
   NODE(parser, OpenMPThreadprivate)
   NODE(parser, OpenStmt)
   NODE(parser, Optional)

flang/include/flang/Parser/parse-tree.h

 // 2.14.1 cancel -> CANCEL construct-type-clause [ [,] if-clause]
 struct OpenMPCancelConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPCancelConstruct);
   WRAPPER_CLASS(If, ScalarLogicalExpr);
   CharBlock source;
   std::tuple<Verbatim, OmpCancelType, std::optional<If>> t;
 };
 
-// 2.13.7 flush -> FLUSH [(variable-name-list)]
+// 2.18.8 Flush Construct [OpenMP 5.0]
+// memory-order-clause -> acq_rel
+//                        release
+//                        acquire
+struct OmpFlushMemoryClause {
+  ENUM_CLASS(FlushMemoryOrder, AcqRel, Release, Acquire)
+  WRAPPER_CLASS_BOILERPLATE(OmpFlushMemoryClause, FlushMemoryOrder);
+  CharBlock source;
+};
+
+// 2.18.8 flush -> FLUSH [memory-order-clause] [(variable-name-list)]
 struct OpenMPFlushConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPFlushConstruct);
   CharBlock source;
-  std::tuple<Verbatim, std::optional<OmpObjectList>> t;
+  std::tuple<Verbatim, std::optional<OmpFlushMemoryClause>,
+      std::optional<OmpObjectList>>
+      t;
 };
 
 struct OmpSimpleStandaloneDirective {
   ENUM_CLASS(Directive, Barrier, Taskwait, Taskyield, TargetEnterData,
       TargetExitData, TargetUpdate, Ordered)
   WRAPPER_CLASS_BOILERPLATE(OmpSimpleStandaloneDirective, Directive);
   CharBlock source;
 };

flang/lib/Parser/openmp-parsers.cpp

 // 2.14.2 Cancellation Point construct
 TYPE_PARSER(sourced(construct<OpenMPCancellationPointConstruct>(
     verbatim("CANCELLATION POINT"_tok), Parser<OmpCancelType>{})))
 
 // 2.14.1 Cancel construct
 TYPE_PARSER(sourced(construct<OpenMPCancelConstruct>(verbatim("CANCEL"_tok),
     Parser<OmpCancelType>{}, maybe("IF" >> parenthesized(scalarLogicalExpr)))))
 
-// 2.13.7 Flush construct
-TYPE_PARSER(sourced(construct<OpenMPFlushConstruct>(
-    verbatim("FLUSH"_tok), maybe(parenthesized(Parser<OmpObjectList>{})))))
+// 2.18.8 Flush construct
+//        flush -> FLUSH [memory-order-clause] [(variable-name-list)]
+//        memory-order-clause -> acq_rel
+//                               release
+//                               acquire
+TYPE_PARSER(sourced(construct<OmpFlushMemoryClause>(
+    "ACQ_REL" >> pure(OmpFlushMemoryClause::FlushMemoryOrder::AcqRel) ||
+    "RELEASE" >> pure(OmpFlushMemoryClause::FlushMemoryOrder::Release) ||
+    "ACQUIRE" >> pure(OmpFlushMemoryClause::FlushMemoryOrder::Acquire))))
+
+TYPE_PARSER(sourced(construct<OpenMPFlushConstruct>(verbatim("FLUSH"_tok),
+    maybe(Parser<OmpFlushMemoryClause>{}),
+    maybe(parenthesized(Parser<OmpObjectList>{})))))
 
 // Simple Standalone Directives
 TYPE_PARSER(sourced(construct<OmpSimpleStandaloneDirective>(first(
     "BARRIER" >> pure(OmpSimpleStandaloneDirective::Directive::Barrier),
     "ORDERED" >> pure(OmpSimpleStandaloneDirective::Directive::Ordered),
     "TARGET ENTER DATA" >>
         pure(OmpSimpleStandaloneDirective::Directive::TargetEnterData),
     "TARGET EXIT DATA" >>

flang/lib/Parser/unparse.cpp

#undef FMT
   void Unparse(const OpenMPCancelConstruct &x) {
     BeginOpenMP();
     Word("!$OMP CANCEL ");
     Walk(std::get<OmpCancelType>(x.t));
     Walk(std::get<std::optional<OpenMPCancelConstruct::If>>(x.t));
     Put("\n");
     EndOpenMP();
   }
+  void Unparse(const OmpFlushMemoryClause &x) {
+    switch (x.v) {
+    case OmpFlushMemoryClause::FlushMemoryOrder::AcqRel:
+      Word("ACQ_REL ");
+      break;
+    case OmpFlushMemoryClause::FlushMemoryOrder::Release:
+      Word("RELEASE ");
+      break;
+    case OmpFlushMemoryClause::FlushMemoryOrder::Acquire:
+      Word("ACQUIRE ");
+      break;
+    }
+  }
   void Unparse(const OpenMPFlushConstruct &x) {
     BeginOpenMP();
-    Word("!$OMP FLUSH");
-    Walk("(", std::get<std::optional<OmpObjectList>>(x.t), ")");
+    Word("!$OMP FLUSH ");
+    Walk(std::get<std::optional<OmpFlushMemoryClause>>(x.t));
+    Walk(" (", std::get<std::optional<OmpObjectList>>(x.t), ")");
     Put("\n");
     EndOpenMP();
   }
   void Unparse(const OmpEndLoopDirective &x) {
     BeginOpenMP();
     Word("!$OMP END ");
     Walk(std::get<OmpLoopDirective>(x.t));
     Walk(std::get<OmpClauseList>(x.t));

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

! Standalone Directives (basic)
   !$omp taskwait
   ! !$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)
   !$omp flush (c)
+  !$omp flush acq_rel
+  !$omp flush release
+  !$omp flush acquire
+  !$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)
   a = 3.14