[flang][OpenMP]Add support for fail clause #118683
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Support the atomic compare option of a fail(memory-order) clauses. Additional tests introduced to check that parsing and semantics checks for the new clause is handled. Lowering for atomic compare is still unsupported and wil end in a TOOD (aka "Not yet implemented"). A test for this case with the fail clause is also present.
llvmbot
added
flang
Member
|
@llvm/pr-subscribers-flang-openmp @llvm/pr-subscribers-flang-semantics Author: Mats Petersson (Leporacanthicus) ChangesSupport the atomic compare option of a fail(memory-order) clauses. Additional tests introduced to check that parsing and semantics checks for the new clause is handled. Lowering for atomic compare is still unsupported and wil end in a TOOD (aka "Not yet implemented"). A test for this case with the fail clause is also present. Full diff: https://github.com/llvm/llvm-project/pull/118683.diff 10 Files Affected:
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index c6f35a07d81ea5..13825eb7ba41e3 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -546,6 +546,7 @@ class ParseTreeDumper {
NODE(parser, OmpEndCriticalDirective)
NODE(parser, OmpEndLoopDirective)
NODE(parser, OmpEndSectionsDirective)
+ NODE(parser, OmpFailClause)
NODE(parser, OmpFromClause)
NODE(OmpFromClause, Modifier)
NODE(parser, OmpExpectation)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8160b095f06dd9..5947f248f6ef77 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -269,6 +269,7 @@ struct OpenACCRoutineConstruct;
struct OpenMPConstruct;
struct OpenMPDeclarativeConstruct;
struct OmpEndLoopDirective;
+struct OmpMemoryOrderClause;
struct CUFKernelDoConstruct;
// Cooked character stream locations
@@ -4098,6 +4099,14 @@ struct OmpUpdateClause {
std::variant<OmpDependenceType, OmpTaskDependenceType> u;
};
+// OMP 5.2 15.8.3 extened-atomic, fail-clause ->
+// FAIL(memory-order)
+struct OmpFailClause {
+ WRAPPER_CLASS_BOILERPLATE(
+ OmpFailClause, common::Indirection<OmpMemoryOrderClause>);
+ CharBlock source;
+};
+
// OpenMP Clauses
struct OmpClause {
UNION_CLASS_BOILERPLATE(OmpClause);
@@ -4317,11 +4326,12 @@ struct OmpMemoryOrderClause {
};
// 2.17.7 Atomic construct
-// atomic-clause -> memory-order-clause | HINT(hint-expression)
+// atomic-clause -> memory-order-clause | HINT(hint-expression) |
+// FAIL(memory-order)
struct OmpAtomicClause {
UNION_CLASS_BOILERPLATE(OmpAtomicClause);
CharBlock source;
- std::variant<OmpMemoryOrderClause, OmpClause> u;
+ std::variant<OmpMemoryOrderClause, OmpFailClause, OmpClause> u;
};
// atomic-clause-list -> [atomic-clause, [atomic-clause], ...]
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 86d475c1a15422..f8fda92d5ac2bb 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -739,6 +739,9 @@ TYPE_PARSER(sourced(construct<OpenMPCancellationPointConstruct>(
TYPE_PARSER(sourced(construct<OpenMPCancelConstruct>(verbatim("CANCEL"_tok),
Parser<OmpCancelType>{}, maybe("IF" >> parenthesized(scalarLogicalExpr)))))
+TYPE_PARSER(sourced(construct<OmpFailClause>(
+ parenthesized(indirect(Parser<OmpMemoryOrderClause>{})))))
+
// 2.17.7 Atomic construct/2.17.8 Flush construct [OpenMP 5.0]
// memory-order-clause ->
// seq_cst
@@ -767,6 +770,7 @@ TYPE_PARSER(construct<OmpAtomicDefaultMemOrderClause>(
// atomic-clause -> memory-order-clause | HINT(hint-expression)
TYPE_PARSER(sourced(construct<OmpAtomicClause>(
construct<OmpAtomicClause>(Parser<OmpMemoryOrderClause>{}) ||
+ construct<OmpAtomicClause>("FAIL" >> Parser<OmpFailClause>{}) ||
construct<OmpAtomicClause>("HINT" >>
sourced(construct<OmpClause>(
construct<OmpClause::Hint>(parenthesized(constantExpr))))))))
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 4782cc1f2d7d7d..a10be3f1c797de 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2702,10 +2702,16 @@ class UnparseVisitor {
Put("\n");
EndOpenMP();
}
+ void Unparse(const OmpFailClause &x) {
+ Word("FAIL(");
+ Walk(x.v);
+ Put(")");
+ }
void Unparse(const OmpMemoryOrderClause &x) { Walk(x.v); }
void Unparse(const OmpAtomicClause &x) {
common::visit(common::visitors{
[&](const OmpMemoryOrderClause &y) { Walk(y); },
+ [&](const OmpFailClause &y) { Walk(y); },
[&](const OmpClause &z) { Walk(z); },
},
x.u);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 27e2b946732abc..4885f79222aa16 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2486,17 +2486,28 @@ void OmpStructureChecker::CheckAtomicMemoryOrderClause(
const parser::OmpAtomicClauseList *leftHandClauseList,
const parser::OmpAtomicClauseList *rightHandClauseList) {
int numMemoryOrderClause = 0;
+ int numFailClause = 0;
auto checkForValidMemoryOrderClause =
[&](const parser::OmpAtomicClauseList *clauseList) {
for (const auto &clause : clauseList->v) {
- if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
- numMemoryOrderClause++;
- if (numMemoryOrderClause > 1) {
+ if (std::get_if<parser::OmpFailClause>(&clause.u)) {
+ numFailClause++;
+ if (numFailClause > 1) {
context_.Say(clause.source,
- "More than one memory order clause not allowed on "
+ "More than one fail clause not allowed on "
"OpenMP Atomic construct"_err_en_US);
return;
}
+ } else {
+ if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
+ numMemoryOrderClause++;
+ if (numMemoryOrderClause > 1) {
+ context_.Say(clause.source,
+ "More than one memory order clause not allowed on "
+ "OpenMP Atomic construct"_err_en_US);
+ return;
+ }
+ }
}
}
};
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index 58dc1f218b56f4..779c24e90e69d9 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -114,6 +114,11 @@ class SemanticsVisitor : public virtual BaseChecker, public virtual C... {
context_.set_location(std::nullopt);
}
+ // This is necessary to avoid "walking" into the Fail clause,
+ // which confuses the CheckAllowed into thinking there's another
+ // memoryorder, when it's actually the argument to the fail clause.
+ bool Pre(const parser::OmpFailClause &) { return false; }
+
bool Walk(const parser::Program &program) {
parser::Walk(program, *this);
return !context_.AnyFatalError();
diff --git a/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90 b/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
new file mode 100644
index 00000000000000..b82bd13622764b
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
@@ -0,0 +1,11 @@
+! RUN: %not_todo_cmd %flang_fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
+
+! CHECK: not yet implemented: OpenMP atomic compare
+program p
+ integer :: x
+ logical :: r
+ !$omp atomic compare fail(relaxed)
+ if (x .eq. 0) then
+ x = 2
+ end if
+end program p
diff --git a/flang/test/Parser/OpenMP/atomic-unparse.f90 b/flang/test/Parser/OpenMP/atomic-unparse.f90
index 64fa79fb1d1a2f..16dc7a1a92bf9e 100644
--- a/flang/test/Parser/OpenMP/atomic-unparse.f90
+++ b/flang/test/Parser/OpenMP/atomic-unparse.f90
@@ -165,6 +165,20 @@ program main
i = j
end if
+
+!$omp atomic compare fail(relaxed)
+ if (i .eq. k) then
+ i = j
+ end if
+!$omp atomic fail(relaxed) compare
+ if (i .eq. k) then
+ i = j
+ end if
+!$omp atomic fail(relaxed) compare acquire
+ if (i .eq. k) then
+ i = j
+ end if
+
!ATOMIC
!$omp atomic
i = j
@@ -262,6 +276,9 @@ end program main
!CHECK: !$OMP ATOMIC COMPARE ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED COMPARE
!CHECK: !$OMP ATOMIC COMPARE RELAXED
+!CHECK: !$OMP ATOMIC COMPARE FAIL(RELAXED)
+!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE
+!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE ACQUIRE
!ATOMIC
!CHECK: !$OMP ATOMIC
@@ -270,3 +287,5 @@ end program main
!CHECK: !$OMP ATOMIC ACQ_REL
!CHECK: !$OMP ATOMIC ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED
+
+
diff --git a/flang/test/Semantics/OpenMP/atomic-compare.f90 b/flang/test/Semantics/OpenMP/atomic-compare.f90
index 85644ad909107e..f677f0c2d8c0f8 100644
--- a/flang/test/Semantics/OpenMP/atomic-compare.f90
+++ b/flang/test/Semantics/OpenMP/atomic-compare.f90
@@ -35,6 +35,14 @@
if (b .eq. a) b = c
!$omp end atomic
+ !$omp atomic hint(1) acq_rel compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
+ !$omp atomic compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
! Check for error conditions:
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one SEQ_CST clause can appear on the COMPARE directive
@@ -75,5 +83,10 @@
!$omp atomic relaxed compare relaxed
if (b .eq. c) b = a
+ !ERROR: More than one fail clause not allowed on OpenMP Atomic construct
+ !$omp atomic fail(release) compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
!$omp end parallel
end
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index bd7fb2361aaeb1..772f60343c6348 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -177,6 +177,7 @@ def OMPC_Exclusive : Clause<"exclusive"> {
}
def OMPC_Fail : Clause<"fail"> {
let clangClass = "OMPFailClause";
+ let flangClass = "OmpFailClause";
}
def OMPC_Filter : Clause<"filter"> {
let clangClass = "OMPFilterClause";
|
Member
|
@llvm/pr-subscribers-flang-parser Author: Mats Petersson (Leporacanthicus) ChangesSupport the atomic compare option of a fail(memory-order) clauses. Additional tests introduced to check that parsing and semantics checks for the new clause is handled. Lowering for atomic compare is still unsupported and wil end in a TOOD (aka "Not yet implemented"). A test for this case with the fail clause is also present. Full diff: https://github.com/llvm/llvm-project/pull/118683.diff 10 Files Affected:
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index c6f35a07d81ea5..13825eb7ba41e3 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -546,6 +546,7 @@ class ParseTreeDumper {
NODE(parser, OmpEndCriticalDirective)
NODE(parser, OmpEndLoopDirective)
NODE(parser, OmpEndSectionsDirective)
+ NODE(parser, OmpFailClause)
NODE(parser, OmpFromClause)
NODE(OmpFromClause, Modifier)
NODE(parser, OmpExpectation)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8160b095f06dd9..5947f248f6ef77 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -269,6 +269,7 @@ struct OpenACCRoutineConstruct;
struct OpenMPConstruct;
struct OpenMPDeclarativeConstruct;
struct OmpEndLoopDirective;
+struct OmpMemoryOrderClause;
struct CUFKernelDoConstruct;
// Cooked character stream locations
@@ -4098,6 +4099,14 @@ struct OmpUpdateClause {
std::variant<OmpDependenceType, OmpTaskDependenceType> u;
};
+// OMP 5.2 15.8.3 extened-atomic, fail-clause ->
+// FAIL(memory-order)
+struct OmpFailClause {
+ WRAPPER_CLASS_BOILERPLATE(
+ OmpFailClause, common::Indirection<OmpMemoryOrderClause>);
+ CharBlock source;
+};
+
// OpenMP Clauses
struct OmpClause {
UNION_CLASS_BOILERPLATE(OmpClause);
@@ -4317,11 +4326,12 @@ struct OmpMemoryOrderClause {
};
// 2.17.7 Atomic construct
-// atomic-clause -> memory-order-clause | HINT(hint-expression)
+// atomic-clause -> memory-order-clause | HINT(hint-expression) |
+// FAIL(memory-order)
struct OmpAtomicClause {
UNION_CLASS_BOILERPLATE(OmpAtomicClause);
CharBlock source;
- std::variant<OmpMemoryOrderClause, OmpClause> u;
+ std::variant<OmpMemoryOrderClause, OmpFailClause, OmpClause> u;
};
// atomic-clause-list -> [atomic-clause, [atomic-clause], ...]
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 86d475c1a15422..f8fda92d5ac2bb 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -739,6 +739,9 @@ TYPE_PARSER(sourced(construct<OpenMPCancellationPointConstruct>(
TYPE_PARSER(sourced(construct<OpenMPCancelConstruct>(verbatim("CANCEL"_tok),
Parser<OmpCancelType>{}, maybe("IF" >> parenthesized(scalarLogicalExpr)))))
+TYPE_PARSER(sourced(construct<OmpFailClause>(
+ parenthesized(indirect(Parser<OmpMemoryOrderClause>{})))))
+
// 2.17.7 Atomic construct/2.17.8 Flush construct [OpenMP 5.0]
// memory-order-clause ->
// seq_cst
@@ -767,6 +770,7 @@ TYPE_PARSER(construct<OmpAtomicDefaultMemOrderClause>(
// atomic-clause -> memory-order-clause | HINT(hint-expression)
TYPE_PARSER(sourced(construct<OmpAtomicClause>(
construct<OmpAtomicClause>(Parser<OmpMemoryOrderClause>{}) ||
+ construct<OmpAtomicClause>("FAIL" >> Parser<OmpFailClause>{}) ||
construct<OmpAtomicClause>("HINT" >>
sourced(construct<OmpClause>(
construct<OmpClause::Hint>(parenthesized(constantExpr))))))))
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 4782cc1f2d7d7d..a10be3f1c797de 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2702,10 +2702,16 @@ class UnparseVisitor {
Put("\n");
EndOpenMP();
}
+ void Unparse(const OmpFailClause &x) {
+ Word("FAIL(");
+ Walk(x.v);
+ Put(")");
+ }
void Unparse(const OmpMemoryOrderClause &x) { Walk(x.v); }
void Unparse(const OmpAtomicClause &x) {
common::visit(common::visitors{
[&](const OmpMemoryOrderClause &y) { Walk(y); },
+ [&](const OmpFailClause &y) { Walk(y); },
[&](const OmpClause &z) { Walk(z); },
},
x.u);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 27e2b946732abc..4885f79222aa16 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2486,17 +2486,28 @@ void OmpStructureChecker::CheckAtomicMemoryOrderClause(
const parser::OmpAtomicClauseList *leftHandClauseList,
const parser::OmpAtomicClauseList *rightHandClauseList) {
int numMemoryOrderClause = 0;
+ int numFailClause = 0;
auto checkForValidMemoryOrderClause =
[&](const parser::OmpAtomicClauseList *clauseList) {
for (const auto &clause : clauseList->v) {
- if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
- numMemoryOrderClause++;
- if (numMemoryOrderClause > 1) {
+ if (std::get_if<parser::OmpFailClause>(&clause.u)) {
+ numFailClause++;
+ if (numFailClause > 1) {
context_.Say(clause.source,
- "More than one memory order clause not allowed on "
+ "More than one fail clause not allowed on "
"OpenMP Atomic construct"_err_en_US);
return;
}
+ } else {
+ if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
+ numMemoryOrderClause++;
+ if (numMemoryOrderClause > 1) {
+ context_.Say(clause.source,
+ "More than one memory order clause not allowed on "
+ "OpenMP Atomic construct"_err_en_US);
+ return;
+ }
+ }
}
}
};
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index 58dc1f218b56f4..779c24e90e69d9 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -114,6 +114,11 @@ class SemanticsVisitor : public virtual BaseChecker, public virtual C... {
context_.set_location(std::nullopt);
}
+ // This is necessary to avoid "walking" into the Fail clause,
+ // which confuses the CheckAllowed into thinking there's another
+ // memoryorder, when it's actually the argument to the fail clause.
+ bool Pre(const parser::OmpFailClause &) { return false; }
+
bool Walk(const parser::Program &program) {
parser::Walk(program, *this);
return !context_.AnyFatalError();
diff --git a/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90 b/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
new file mode 100644
index 00000000000000..b82bd13622764b
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
@@ -0,0 +1,11 @@
+! RUN: %not_todo_cmd %flang_fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
+
+! CHECK: not yet implemented: OpenMP atomic compare
+program p
+ integer :: x
+ logical :: r
+ !$omp atomic compare fail(relaxed)
+ if (x .eq. 0) then
+ x = 2
+ end if
+end program p
diff --git a/flang/test/Parser/OpenMP/atomic-unparse.f90 b/flang/test/Parser/OpenMP/atomic-unparse.f90
index 64fa79fb1d1a2f..16dc7a1a92bf9e 100644
--- a/flang/test/Parser/OpenMP/atomic-unparse.f90
+++ b/flang/test/Parser/OpenMP/atomic-unparse.f90
@@ -165,6 +165,20 @@ program main
i = j
end if
+
+!$omp atomic compare fail(relaxed)
+ if (i .eq. k) then
+ i = j
+ end if
+!$omp atomic fail(relaxed) compare
+ if (i .eq. k) then
+ i = j
+ end if
+!$omp atomic fail(relaxed) compare acquire
+ if (i .eq. k) then
+ i = j
+ end if
+
!ATOMIC
!$omp atomic
i = j
@@ -262,6 +276,9 @@ end program main
!CHECK: !$OMP ATOMIC COMPARE ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED COMPARE
!CHECK: !$OMP ATOMIC COMPARE RELAXED
+!CHECK: !$OMP ATOMIC COMPARE FAIL(RELAXED)
+!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE
+!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE ACQUIRE
!ATOMIC
!CHECK: !$OMP ATOMIC
@@ -270,3 +287,5 @@ end program main
!CHECK: !$OMP ATOMIC ACQ_REL
!CHECK: !$OMP ATOMIC ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED
+
+
diff --git a/flang/test/Semantics/OpenMP/atomic-compare.f90 b/flang/test/Semantics/OpenMP/atomic-compare.f90
index 85644ad909107e..f677f0c2d8c0f8 100644
--- a/flang/test/Semantics/OpenMP/atomic-compare.f90
+++ b/flang/test/Semantics/OpenMP/atomic-compare.f90
@@ -35,6 +35,14 @@
if (b .eq. a) b = c
!$omp end atomic
+ !$omp atomic hint(1) acq_rel compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
+ !$omp atomic compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
! Check for error conditions:
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one SEQ_CST clause can appear on the COMPARE directive
@@ -75,5 +83,10 @@
!$omp atomic relaxed compare relaxed
if (b .eq. c) b = a
+ !ERROR: More than one fail clause not allowed on OpenMP Atomic construct
+ !$omp atomic fail(release) compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
!$omp end parallel
end
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index bd7fb2361aaeb1..772f60343c6348 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -177,6 +177,7 @@ def OMPC_Exclusive : Clause<"exclusive"> {
}
def OMPC_Fail : Clause<"fail"> {
let clangClass = "OMPFailClause";
+ let flangClass = "OmpFailClause";
}
def OMPC_Filter : Clause<"filter"> {
let clangClass = "OMPFilterClause";
|
Member
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Mats Petersson (Leporacanthicus) ChangesSupport the atomic compare option of a fail(memory-order) clauses. Additional tests introduced to check that parsing and semantics checks for the new clause is handled. Lowering for atomic compare is still unsupported and wil end in a TOOD (aka "Not yet implemented"). A test for this case with the fail clause is also present. Full diff: https://github.com/llvm/llvm-project/pull/118683.diff 10 Files Affected:
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index c6f35a07d81ea5..13825eb7ba41e3 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -546,6 +546,7 @@ class ParseTreeDumper {
NODE(parser, OmpEndCriticalDirective)
NODE(parser, OmpEndLoopDirective)
NODE(parser, OmpEndSectionsDirective)
+ NODE(parser, OmpFailClause)
NODE(parser, OmpFromClause)
NODE(OmpFromClause, Modifier)
NODE(parser, OmpExpectation)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index 8160b095f06dd9..5947f248f6ef77 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -269,6 +269,7 @@ struct OpenACCRoutineConstruct;
struct OpenMPConstruct;
struct OpenMPDeclarativeConstruct;
struct OmpEndLoopDirective;
+struct OmpMemoryOrderClause;
struct CUFKernelDoConstruct;
// Cooked character stream locations
@@ -4098,6 +4099,14 @@ struct OmpUpdateClause {
std::variant<OmpDependenceType, OmpTaskDependenceType> u;
};
+// OMP 5.2 15.8.3 extened-atomic, fail-clause ->
+// FAIL(memory-order)
+struct OmpFailClause {
+ WRAPPER_CLASS_BOILERPLATE(
+ OmpFailClause, common::Indirection<OmpMemoryOrderClause>);
+ CharBlock source;
+};
+
// OpenMP Clauses
struct OmpClause {
UNION_CLASS_BOILERPLATE(OmpClause);
@@ -4317,11 +4326,12 @@ struct OmpMemoryOrderClause {
};
// 2.17.7 Atomic construct
-// atomic-clause -> memory-order-clause | HINT(hint-expression)
+// atomic-clause -> memory-order-clause | HINT(hint-expression) |
+// FAIL(memory-order)
struct OmpAtomicClause {
UNION_CLASS_BOILERPLATE(OmpAtomicClause);
CharBlock source;
- std::variant<OmpMemoryOrderClause, OmpClause> u;
+ std::variant<OmpMemoryOrderClause, OmpFailClause, OmpClause> u;
};
// atomic-clause-list -> [atomic-clause, [atomic-clause], ...]
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 86d475c1a15422..f8fda92d5ac2bb 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -739,6 +739,9 @@ TYPE_PARSER(sourced(construct<OpenMPCancellationPointConstruct>(
TYPE_PARSER(sourced(construct<OpenMPCancelConstruct>(verbatim("CANCEL"_tok),
Parser<OmpCancelType>{}, maybe("IF" >> parenthesized(scalarLogicalExpr)))))
+TYPE_PARSER(sourced(construct<OmpFailClause>(
+ parenthesized(indirect(Parser<OmpMemoryOrderClause>{})))))
+
// 2.17.7 Atomic construct/2.17.8 Flush construct [OpenMP 5.0]
// memory-order-clause ->
// seq_cst
@@ -767,6 +770,7 @@ TYPE_PARSER(construct<OmpAtomicDefaultMemOrderClause>(
// atomic-clause -> memory-order-clause | HINT(hint-expression)
TYPE_PARSER(sourced(construct<OmpAtomicClause>(
construct<OmpAtomicClause>(Parser<OmpMemoryOrderClause>{}) ||
+ construct<OmpAtomicClause>("FAIL" >> Parser<OmpFailClause>{}) ||
construct<OmpAtomicClause>("HINT" >>
sourced(construct<OmpClause>(
construct<OmpClause::Hint>(parenthesized(constantExpr))))))))
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 4782cc1f2d7d7d..a10be3f1c797de 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2702,10 +2702,16 @@ class UnparseVisitor {
Put("\n");
EndOpenMP();
}
+ void Unparse(const OmpFailClause &x) {
+ Word("FAIL(");
+ Walk(x.v);
+ Put(")");
+ }
void Unparse(const OmpMemoryOrderClause &x) { Walk(x.v); }
void Unparse(const OmpAtomicClause &x) {
common::visit(common::visitors{
[&](const OmpMemoryOrderClause &y) { Walk(y); },
+ [&](const OmpFailClause &y) { Walk(y); },
[&](const OmpClause &z) { Walk(z); },
},
x.u);
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 27e2b946732abc..4885f79222aa16 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -2486,17 +2486,28 @@ void OmpStructureChecker::CheckAtomicMemoryOrderClause(
const parser::OmpAtomicClauseList *leftHandClauseList,
const parser::OmpAtomicClauseList *rightHandClauseList) {
int numMemoryOrderClause = 0;
+ int numFailClause = 0;
auto checkForValidMemoryOrderClause =
[&](const parser::OmpAtomicClauseList *clauseList) {
for (const auto &clause : clauseList->v) {
- if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
- numMemoryOrderClause++;
- if (numMemoryOrderClause > 1) {
+ if (std::get_if<parser::OmpFailClause>(&clause.u)) {
+ numFailClause++;
+ if (numFailClause > 1) {
context_.Say(clause.source,
- "More than one memory order clause not allowed on "
+ "More than one fail clause not allowed on "
"OpenMP Atomic construct"_err_en_US);
return;
}
+ } else {
+ if (std::get_if<Fortran::parser::OmpMemoryOrderClause>(&clause.u)) {
+ numMemoryOrderClause++;
+ if (numMemoryOrderClause > 1) {
+ context_.Say(clause.source,
+ "More than one memory order clause not allowed on "
+ "OpenMP Atomic construct"_err_en_US);
+ return;
+ }
+ }
}
}
};
diff --git a/flang/lib/Semantics/semantics.cpp b/flang/lib/Semantics/semantics.cpp
index 58dc1f218b56f4..779c24e90e69d9 100644
--- a/flang/lib/Semantics/semantics.cpp
+++ b/flang/lib/Semantics/semantics.cpp
@@ -114,6 +114,11 @@ class SemanticsVisitor : public virtual BaseChecker, public virtual C... {
context_.set_location(std::nullopt);
}
+ // This is necessary to avoid "walking" into the Fail clause,
+ // which confuses the CheckAllowed into thinking there's another
+ // memoryorder, when it's actually the argument to the fail clause.
+ bool Pre(const parser::OmpFailClause &) { return false; }
+
bool Walk(const parser::Program &program) {
parser::Walk(program, *this);
return !context_.AnyFatalError();
diff --git a/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90 b/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
new file mode 100644
index 00000000000000..b82bd13622764b
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/atomic-compare-fail.f90
@@ -0,0 +1,11 @@
+! RUN: %not_todo_cmd %flang_fc1 -emit-fir -fopenmp -fopenmp-version=51 -o - %s 2>&1 | FileCheck %s
+
+! CHECK: not yet implemented: OpenMP atomic compare
+program p
+ integer :: x
+ logical :: r
+ !$omp atomic compare fail(relaxed)
+ if (x .eq. 0) then
+ x = 2
+ end if
+end program p
diff --git a/flang/test/Parser/OpenMP/atomic-unparse.f90 b/flang/test/Parser/OpenMP/atomic-unparse.f90
index 64fa79fb1d1a2f..16dc7a1a92bf9e 100644
--- a/flang/test/Parser/OpenMP/atomic-unparse.f90
+++ b/flang/test/Parser/OpenMP/atomic-unparse.f90
@@ -165,6 +165,20 @@ program main
i = j
end if
+
+!$omp atomic compare fail(relaxed)
+ if (i .eq. k) then
+ i = j
+ end if
+!$omp atomic fail(relaxed) compare
+ if (i .eq. k) then
+ i = j
+ end if
+!$omp atomic fail(relaxed) compare acquire
+ if (i .eq. k) then
+ i = j
+ end if
+
!ATOMIC
!$omp atomic
i = j
@@ -262,6 +276,9 @@ end program main
!CHECK: !$OMP ATOMIC COMPARE ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED COMPARE
!CHECK: !$OMP ATOMIC COMPARE RELAXED
+!CHECK: !$OMP ATOMIC COMPARE FAIL(RELAXED)
+!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE
+!CHECK: !$OMP ATOMIC FAIL(RELAXED) COMPARE ACQUIRE
!ATOMIC
!CHECK: !$OMP ATOMIC
@@ -270,3 +287,5 @@ end program main
!CHECK: !$OMP ATOMIC ACQ_REL
!CHECK: !$OMP ATOMIC ACQUIRE
!CHECK: !$OMP ATOMIC RELAXED
+
+
diff --git a/flang/test/Semantics/OpenMP/atomic-compare.f90 b/flang/test/Semantics/OpenMP/atomic-compare.f90
index 85644ad909107e..f677f0c2d8c0f8 100644
--- a/flang/test/Semantics/OpenMP/atomic-compare.f90
+++ b/flang/test/Semantics/OpenMP/atomic-compare.f90
@@ -35,6 +35,14 @@
if (b .eq. a) b = c
!$omp end atomic
+ !$omp atomic hint(1) acq_rel compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
+ !$omp atomic compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
! Check for error conditions:
!ERROR: More than one memory order clause not allowed on OpenMP Atomic construct
!ERROR: At most one SEQ_CST clause can appear on the COMPARE directive
@@ -75,5 +83,10 @@
!$omp atomic relaxed compare relaxed
if (b .eq. c) b = a
+ !ERROR: More than one fail clause not allowed on OpenMP Atomic construct
+ !$omp atomic fail(release) compare fail(release)
+ if (c .eq. a) a = b
+ !$omp end atomic
+
!$omp end parallel
end
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.td b/llvm/include/llvm/Frontend/OpenMP/OMP.td
index bd7fb2361aaeb1..772f60343c6348 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.td
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.td
@@ -177,6 +177,7 @@ def OMPC_Exclusive : Clause<"exclusive"> {
}
def OMPC_Fail : Clause<"fail"> {
let clangClass = "OMPFailClause";
+ let flangClass = "OmpFailClause";
}
def OMPC_Filter : Clause<"filter"> {
let clangClass = "OMPFilterClause";
|
Leporacanthicus
requested review from
DavidTruby,
TIFitis,
kiranchandramohan,
kparzysz and
tblah
and removed request for
tblah
kparzysz
reviewed
Dec 4, 2024
Contributor
kparzysz
left a comment
kparzysz
left a comment
There was a problem hiding this comment.
Looks good with a few minor comments.
Comment on lines
4102
to
4109
| // OMP 5.2 15.8.3 extened-atomic, fail-clause -> | ||
| // FAIL(memory-order) | ||
| struct OmpFailClause { | ||
| WRAPPER_CLASS_BOILERPLATE( | ||
| OmpFailClause, common::Indirection<OmpMemoryOrderClause>); | ||
| CharBlock source; | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
The OmpXyzClause classes are ordered alphabetically. Could you move it to the right location?
| if (numFailClause > 1) { | ||
| context_.Say(clause.source, | ||
| "More than one memory order clause not allowed on " | ||
| "More than one fail clause not allowed on " |
Contributor
There was a problem hiding this comment.
Could you spell FAIL in all capital letters? This is the convention in diagnostic messages.
Also, could you concatenate the strings into a single line. The line will be "too long", but we do that to make grepping for messages in the sources easier.
Finally, please replace the "OpenMP Atomic construct" with "ATOMIC construct".
Contributor
Author
There was a problem hiding this comment.
Good catch on all three parts. This was a bit of a "copy-paste" thing, so I just followed what was already there for memory order, but of course, memory order isn't a "keyword" the same way that FAIL is. The split strings is annoying me quite often, so thanks for pointing that out - again, copying what was already there (poor excuse, but it's the best I've got!)
| if (numMemoryOrderClause > 1) { | ||
| context_.Say(clause.source, | ||
| "More than one memory order clause not allowed on " | ||
| "OpenMP Atomic construct"_err_en_US); |
Contributor
There was a problem hiding this comment.
Same comment here---please concatenate and use "ATOMIC construct".
| const parser::OmpAtomicClauseList *leftHandClauseList, | ||
| const parser::OmpAtomicClauseList *rightHandClauseList) { | ||
| int numMemoryOrderClause = 0; | ||
| int numFailClause = 0; |
Contributor
There was a problem hiding this comment.
Nit: braced initialization in frontend code.
| WRAPPER_CLASS_BOILERPLATE(OmpDeviceTypeClause, DeviceTypeDescription); | ||
| }; | ||
|
|
||
| // OMP 5.2 15.8.3 extened-atomic, fail-clause -> |
Contributor
There was a problem hiding this comment.
Suggested change
| // OMP 5.2 15.8.3 extened-atomic, fail-clause -> | |
| // OMP 5.2 15.8.3 extended-atomic, fail-clause -> |
flang/lib/Semantics/semantics.cpp
Outdated
Comment on lines
117
to
120
| // This is necessary to avoid "walking" into the Fail clause, | ||
| // which confuses the CheckAllowed into thinking there's another | ||
| // memoryorder, when it's actually the argument to the fail clause. | ||
| bool Pre(const parser::OmpFailClause &) { return false; } |
Contributor
There was a problem hiding this comment.
Would this disallow any future checks for the OmpFailClause?
What are the alternatives here?
Marking the MemoryOrder values for OmpFail as an ENUM or something?
Alternative, would be to manually check the clauses having MemoryOrder components in check-omp-structure.cpp.
Contributor
Author
There was a problem hiding this comment.
Assuming we have the correct parser, there is no way that the FAIL clause will contain anything other than a MemoryOrder.
The problem we're trying to solve here is that the default is to Walk everything where Pre returns true. So if we have !$OMP ATOMIC COMPARE SEQ_CST FAIL(RELAXED), it will get the the FAIL, and default Pre returns true, so it gets to process the RELAXED. It treats that as if it was part of the regular list of memory order objects listed, and says "There's two memory-orders on the same ATOMIC COMPARE" (not the precise wording).
We COULD use a different type, and then have a separate checker bit, but than we will also end up with two (almost) identical parsers for memory order objects, and I'm not sure exactly what it solves to do that.
We can always call some function inside Pre that checks things. Or call check the fail clause in the check-omp-structure.cpp.
Really, none of the solutions are fantastic. This works for the moment, and I'm not sure it makes sense to add more complexity, unless we have a good reason for that.
Contributor
There was a problem hiding this comment.
I was asking whether this solution will prevent the ability to perform the following semantic check from the OpenMP 5.2 standard.
-> acq_rel and release cannot be specified as arguments to the fail clause.
If you do not want to change the parse-tree representation, an alternative solution could be to remember whether we are visiting the FailClause and only call CheckAllowedClause for the MemoryOrder clauses if we are not inside the FailClause. Something like the following (only done for Release MemoryOrder clause).
+void OmpStructureChecker::Enter(const parser::OmpFailClause &) {
+ CheckAllowedClause(llvm::omp::Clause::OMPC_fail);
+ isFailClause = true;
+}
+
+void OmpStructureChecker::Leave(const parser::OmpFailClause &) {
+ isFailClause = false;
+}
+
+void OmpStructureChecker::Enter(const parser::OmpClause::Release &) {
+ if(!isFailClause) {
+ CheckAllowedClause(llvm::omp::Clause::OMPC_release);
+ }
+}
Note: The above code will also involve removing the CHECK_SIMPLE_CLAUSE entries for the Release MemoryOrder clause, adding isFailClause as a member of OmpStructureChecker, and adding the Enter and Leave signatures in OmpStructureChecker for the OmpFailClause.
Contributor
Author
There was a problem hiding this comment.
Ah, I see. As far as I can tell, there's nothing automatic (in OMP.td) that provides a help for the fail clause's memory order options. We'd have to write code for that somewhere. We already do a bunch of checking for memoryorder in the semantics of ATOMIC in general, and we also need to check that the fail clause doesn't appear outside of a COMPARE (at least I think that's how it's meant to work).
The alternative suggested would be a workable solution. In fact, I'm liking that more, mostly because it doesn't alter a generic class used everywhere. I'll implement that.
In both cases, we still need to know whether we're checking a memory order inside a fail clause or not - and I guess we would add an else Say("Release memory order not allowed on FAIL clause") in the not allowed cases (not exactly how it would be written, but conceptually).
kiranchandramohan
approved these changes
Dec 10, 2024
Contributor
kiranchandramohan
left a comment
kiranchandramohan
left a comment
There was a problem hiding this comment.
LG. Thanks for the changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Support the atomic compare option of a fail(memory-order) clauses.
Additional tests introduced to check that parsing and semantics checks for the new clause is handled.
Lowering for atomic compare is still unsupported and wil end in a TOOD (aka "Not yet implemented"). A test for this case with the fail clause is also present.