[flang][OpenMP] Update handling of DEPEND clause #113620
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
Parse the locator list in OmpDependClause as an OmpObjectList (instead of a list of Designators). When a common block appears in the locator list, show an informative message. Implement resolving symbols in DependSinkVec in a dedicated visitor instead of having a visitor for OmpDependClause. Resolve unresolved names common blocks in OmpObjectList. Minor changes to the code organization: - rename OmpDependenceType to OmpTaskDependenceType (to follow 5.2 terminology), - rename Depend::WithLocators to Depend::DepType, - add comments with more detailed spec references to parse-tree.h.
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-parser Author: Krzysztof Parzyszek (kparzysz) ChangesParse the locator list in OmpDependClause as an OmpObjectList (instead of a list of Designators). When a common block appears in the locator list, show an informative message. Minor changes to the code organization:
Full diff: https://github.com/llvm/llvm-project/pull/113620.diff 13 Files Affected:
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
index 5d3c5cd72eef04..d28ed0534d6002 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp
@@ -222,9 +222,9 @@ void OpenMPCounterVisitor::Post(const OmpLinearModifier::Type &c) {
clauseDetails +=
"modifier=" + std::string{OmpLinearModifier::EnumToString(c)} + ";";
}
-void OpenMPCounterVisitor::Post(const OmpDependenceType::Type &c) {
+void OpenMPCounterVisitor::Post(const OmpTaskDependenceType::Type &c) {
clauseDetails +=
- "type=" + std::string{OmpDependenceType::EnumToString(c)} + ";";
+ "type=" + std::string{OmpTaskDependenceType::EnumToString(c)} + ";";
}
void OpenMPCounterVisitor::Post(const OmpMapClause::Type &c) {
clauseDetails += "type=" + std::string{OmpMapClause::EnumToString(c)} + ";";
diff --git a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
index 380534ebbfd70a..68c52db46e2f00 100644
--- a/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
+++ b/flang/examples/FlangOmpReport/FlangOmpReportVisitor.h
@@ -73,7 +73,7 @@ struct OpenMPCounterVisitor {
void Post(const OmpDeviceTypeClause::Type &c);
void Post(const OmpScheduleModifierType::ModType &c);
void Post(const OmpLinearModifier::Type &c);
- void Post(const OmpDependenceType::Type &c);
+ void Post(const OmpTaskDependenceType::Type &c);
void Post(const OmpMapClause::Type &c);
void Post(const OmpScheduleClause::ScheduleType &c);
void Post(const OmpIfClause::DirectiveNameModifier &c);
diff --git a/flang/include/flang/Parser/dump-parse-tree.h b/flang/include/flang/Parser/dump-parse-tree.h
index 76d2f164fc4bf0..1517539ff85f3b 100644
--- a/flang/include/flang/Parser/dump-parse-tree.h
+++ b/flang/include/flang/Parser/dump-parse-tree.h
@@ -513,8 +513,8 @@ class ParseTreeDumper {
NODE(OmpDependClause, InOut)
NODE(OmpDependClause, Sink)
NODE(OmpDependClause, Source)
- NODE(parser, OmpDependenceType)
- NODE_ENUM(OmpDependenceType, Type)
+ NODE(parser, OmpTaskDependenceType)
+ NODE_ENUM(OmpTaskDependenceType, Type)
NODE(parser, OmpDependSinkVec)
NODE(parser, OmpDependSinkVecLength)
NODE(parser, OmpEndAllocators)
diff --git a/flang/include/flang/Parser/parse-tree.h b/flang/include/flang/Parser/parse-tree.h
index c1884f6e88d1ec..25e2f42ca29bd5 100644
--- a/flang/include/flang/Parser/parse-tree.h
+++ b/flang/include/flang/Parser/parse-tree.h
@@ -3439,6 +3439,18 @@ struct OmpObject {
WRAPPER_CLASS(OmpObjectList, std::list<OmpObject>);
+// Ref: [4.5:169-170], [5.0:254-256], [5.1:287-289], [5.2:321]
+//
+// task-dependence-type -> // "dependence-type" in 5.1 and before
+// IN | OUT | INOUT | // since 4.5
+// SOURCE | SINK | // since 4.5, until 5.1
+// MUTEXINOUTSET | DEPOBJ | // since 5.0
+// INOUTSET // since 5.2
+struct OmpTaskDependenceType {
+ ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
+ WRAPPER_CLASS_BOILERPLATE(OmpTaskDependenceType, Type);
+};
+
// [5.0] 2.1.6 iterator-specifier -> type-declaration-stmt = subscript-triple
// iterator-modifier -> iterator-specifier-list
struct OmpIteratorSpecifier {
@@ -3534,27 +3546,27 @@ struct OmpDependSinkVecLength {
std::tuple<DefinedOperator, ScalarIntConstantExpr> t;
};
-// 2.13.9 depend-vec -> iterator [+/- depend-vec-length],...,iterator[...]
+// 2.13.9 depend-vec -> induction-variable [depend-vec-length], ...
struct OmpDependSinkVec {
TUPLE_CLASS_BOILERPLATE(OmpDependSinkVec);
std::tuple<Name, std::optional<OmpDependSinkVecLength>> t;
};
-// 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);
-};
-
-// 2.13.9 depend-clause -> DEPEND (((IN | OUT | INOUT) : variable-name-list) |
-// SOURCE | SINK : depend-vec)
+// Ref: [4.5:169-170], [5.0:255-256], [5.1:288-289], [5.2:323-324]
+//
+// depend-clause ->
+// DEPEND(SOURCE) | // since 4.5, until 5.1
+// DEPEND(SINK: depend-vec) | // since 4.5, until 5.1
+// DEPEND([depend-modifier,]dependence-type: locator-list) // since 4.5
+//
+// depend-modifier -> iterator-modifier // since 5.0
struct OmpDependClause {
UNION_CLASS_BOILERPLATE(OmpDependClause);
EMPTY_CLASS(Source);
WRAPPER_CLASS(Sink, std::list<OmpDependSinkVec>);
struct InOut {
TUPLE_CLASS_BOILERPLATE(InOut);
- std::tuple<OmpDependenceType, std::list<Designator>> t;
+ std::tuple<OmpTaskDependenceType, OmpObjectList> t;
};
std::variant<Source, Sink, InOut> u;
};
diff --git a/flang/include/flang/Semantics/symbol.h b/flang/include/flang/Semantics/symbol.h
index 0767d8ea84bc6b..db1c25285c080f 100644
--- a/flang/include/flang/Semantics/symbol.h
+++ b/flang/include/flang/Semantics/symbol.h
@@ -24,6 +24,8 @@
#include <set>
#include <vector>
+#include "llvm/Support/Signals.h"
+
namespace llvm {
class raw_ostream;
}
@@ -753,9 +755,9 @@ class Symbol {
// OpenMP miscellaneous flags
OmpCommonBlock, OmpReduction, OmpAligned, OmpNontemporal, OmpAllocate,
OmpDeclarativeAllocateDirective, OmpExecutableAllocateDirective,
- OmpDeclareSimd, OmpDeclareTarget, OmpThreadprivate, OmpDeclareReduction,
- OmpFlushed, OmpCriticalLock, OmpIfSpecified, OmpNone, OmpPreDetermined,
- OmpImplicit);
+ OmpDependClause, OmpDeclareSimd, OmpDeclareTarget, OmpThreadprivate,
+ OmpDeclareReduction, OmpFlushed, OmpCriticalLock, OmpIfSpecified, OmpNone,
+ OmpPreDetermined, OmpImplicit);
using Flags = common::EnumSet<Flag, Flag_enumSize>;
const Scope &owner() const { return *owner_; }
@@ -976,6 +978,7 @@ template <std::size_t BLOCK_SIZE> class Symbols {
}
private:
+ static int counter;
using blockType = std::array<Symbol, BLOCK_SIZE>;
std::list<blockType *> blocks_;
std::size_t nextIndex_{0};
@@ -994,6 +997,8 @@ template <std::size_t BLOCK_SIZE> class Symbols {
}
};
+template <std::size_t BLOCK_SIZE> int Symbols<BLOCK_SIZE>::counter;
+
// Define a few member functions here in the header so that they
// can be used by lib/Evaluate without inducing a dependence cycle
// between the two shared libraries.
diff --git a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
index fbc031f3a93d7d..12fe3b572244f0 100644
--- a/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
+++ b/flang/lib/Lower/OpenMP/ClauseProcessor.cpp
@@ -798,11 +798,11 @@ bool ClauseProcessor::processDepend(mlir::omp::DependClauseOps &result) const {
return findRepeatableClause<omp::clause::Depend>(
[&](const omp::clause::Depend &clause, const parser::CharBlock &) {
using Depend = omp::clause::Depend;
- assert(std::holds_alternative<Depend::WithLocators>(clause.u) &&
- "Only the modern form is handled at the moment");
- auto &modern = std::get<Depend::WithLocators>(clause.u);
- auto kind = std::get<Depend::TaskDependenceType>(modern.t);
- auto &objects = std::get<omp::ObjectList>(modern.t);
+ assert(std::holds_alternative<Depend::DepType>(clause.u) &&
+ "Only the form with depenence type is handled at the moment");
+ auto &depType = std::get<Depend::DepType>(clause.u);
+ auto kind = std::get<Depend::TaskDependenceType>(depType.t);
+ auto &objects = std::get<omp::ObjectList>(depType.t);
mlir::omp::ClauseTaskDependAttr dependTypeOperand =
genDependKindAttr(firOpBuilder, kind);
diff --git a/flang/lib/Lower/OpenMP/Clauses.cpp b/flang/lib/Lower/OpenMP/Clauses.cpp
index ee3d74a7c631af..57667fbbf7d4aa 100644
--- a/flang/lib/Lower/OpenMP/Clauses.cpp
+++ b/flang/lib/Lower/OpenMP/Clauses.cpp
@@ -555,7 +555,7 @@ Depend make(const parser::OmpClause::Depend &inp,
using Iteration = Doacross::Vector::value_type; // LoopIterationT
CLAUSET_ENUM_CONVERT( //
- convert1, parser::OmpDependenceType::Type, Depend::TaskDependenceType,
+ convert1, parser::OmpTaskDependenceType::Type, Depend::TaskDependenceType,
// clang-format off
MS(In, In)
MS(Out, Out)
@@ -593,17 +593,14 @@ Depend make(const parser::OmpClause::Depend &inp,
return Doacross{{/*DependenceType=*/Doacross::DependenceType::Sink,
/*Vector=*/makeList(s.v, convert2)}};
},
- // Depend::WithLocators
+ // Depend::DepType
[&](const wrapped::InOut &s) -> Variant {
- auto &t0 = std::get<parser::OmpDependenceType>(s.t);
- auto &t1 = std::get<std::list<parser::Designator>>(s.t);
- auto convert4 = [&](const parser::Designator &t) {
- return makeObject(t, semaCtx);
- };
- return Depend::WithLocators{
+ auto &t0 = std::get<parser::OmpTaskDependenceType>(s.t);
+ auto &t1 = std::get<parser::OmpObjectList>(s.t);
+ return Depend::DepType{
{/*TaskDependenceType=*/convert1(t0.v),
/*Iterator=*/std::nullopt,
- /*LocatorList=*/makeList(t1, convert4)}};
+ /*LocatorList=*/makeObjects(t1, semaCtx)}};
},
},
inp.v.u)};
diff --git a/flang/lib/Parser/openmp-parsers.cpp b/flang/lib/Parser/openmp-parsers.cpp
index 59a8757e58e8cc..3a19d44559dccd 100644
--- a/flang/lib/Parser/openmp-parsers.cpp
+++ b/flang/lib/Parser/openmp-parsers.cpp
@@ -365,10 +365,10 @@ TYPE_PARSER(construct<OmpDependSinkVecLength>(
TYPE_PARSER(
construct<OmpDependSinkVec>(name, maybe(Parser<OmpDependSinkVecLength>{})))
-TYPE_PARSER(
- construct<OmpDependenceType>("IN"_id >> pure(OmpDependenceType::Type::In) ||
- "INOUT" >> pure(OmpDependenceType::Type::Inout) ||
- "OUT" >> pure(OmpDependenceType::Type::Out)))
+TYPE_PARSER(construct<OmpTaskDependenceType>(
+ "IN"_id >> pure(OmpTaskDependenceType::Type::In) ||
+ "INOUT" >> pure(OmpTaskDependenceType::Type::Inout) ||
+ "OUT" >> pure(OmpTaskDependenceType::Type::Out)))
TYPE_CONTEXT_PARSER("Omp Depend clause"_en_US,
construct<OmpDependClause>(construct<OmpDependClause::Sink>(
@@ -376,7 +376,7 @@ TYPE_CONTEXT_PARSER("Omp Depend clause"_en_US,
construct<OmpDependClause>(
construct<OmpDependClause::Source>("SOURCE"_tok)) ||
construct<OmpDependClause>(construct<OmpDependClause::InOut>(
- Parser<OmpDependenceType>{}, ":" >> nonemptyList(designator))))
+ Parser<OmpTaskDependenceType>{}, ":" >> Parser<OmpObjectList>{})))
// 2.15.3.7 LINEAR (linear-list: linear-step)
// linear-list -> list | modifier(list)
diff --git a/flang/lib/Parser/unparse.cpp b/flang/lib/Parser/unparse.cpp
index 04df988223e8f8..1445468b4fb73f 100644
--- a/flang/lib/Parser/unparse.cpp
+++ b/flang/lib/Parser/unparse.cpp
@@ -2206,9 +2206,9 @@ class UnparseVisitor {
}
void Unparse(const OmpDependClause::InOut &x) {
Put("(");
- Walk(std::get<OmpDependenceType>(x.t));
+ Walk(std::get<OmpTaskDependenceType>(x.t));
Put(":");
- Walk(std::get<std::list<Designator>>(x.t), ",");
+ Walk(std::get<OmpObjectList>(x.t));
Put(")");
}
bool Pre(const OmpDependClause &x) {
@@ -2816,7 +2816,7 @@ class UnparseVisitor {
OmpLastprivateClause, LastprivateModifier) // OMP lastprivate-modifier
WALK_NESTED_ENUM(OmpScheduleModifierType, ModType) // OMP schedule-modifier
WALK_NESTED_ENUM(OmpLinearModifier, Type) // OMP linear-modifier
- WALK_NESTED_ENUM(OmpDependenceType, Type) // OMP dependence-type
+ WALK_NESTED_ENUM(OmpTaskDependenceType, Type) // OMP task-dependence-type
WALK_NESTED_ENUM(OmpScheduleClause, ScheduleType) // OMP schedule-type
WALK_NESTED_ENUM(OmpDeviceClause, DeviceModifier) // OMP device modifier
WALK_NESTED_ENUM(OmpDeviceTypeClause, Type) // OMP DEVICE_TYPE
diff --git a/flang/lib/Semantics/check-omp-structure.cpp b/flang/lib/Semantics/check-omp-structure.cpp
index 46486907ceb9e1..892745c9c36b6b 100644
--- a/flang/lib/Semantics/check-omp-structure.cpp
+++ b/flang/lib/Semantics/check-omp-structure.cpp
@@ -3283,15 +3283,21 @@ void OmpStructureChecker::Enter(const parser::OmpClause::Depend &x) {
parser::ToUpperCaseLetters(getDirectiveName(GetContext().directive)));
}
if (const auto *inOut{std::get_if<parser::OmpDependClause::InOut>(&x.v.u)}) {
- const auto &designators{std::get<std::list<parser::Designator>>(inOut->t)};
- for (const auto &ele : designators) {
- if (const auto *dataRef{std::get_if<parser::DataRef>(&ele.u)}) {
- CheckDependList(*dataRef);
- if (const auto *arr{
- std::get_if<common::Indirection<parser::ArrayElement>>(
- &dataRef->u)}) {
- CheckArraySection(arr->value(), GetLastName(*dataRef),
- llvm::omp::Clause::OMPC_depend);
+ for (const auto &object : std::get<parser::OmpObjectList>(inOut->t).v) {
+ if (const auto *name{std::get_if<parser::Name>(&object.u)}) {
+ context_.Say(GetContext().clauseSource,
+ "Common block name ('%s') cannot appear in a DEPEND "
+ "clause"_err_en_US,
+ name->ToString());
+ } else if (auto *designator{std::get_if<parser::Designator>(&object.u)}) {
+ if (auto *dataRef{std::get_if<parser::DataRef>(&designator->u)}) {
+ CheckDependList(*dataRef);
+ if (const auto *arr{
+ std::get_if<common::Indirection<parser::ArrayElement>>(
+ &dataRef->u)}) {
+ CheckArraySection(arr->value(), GetLastName(*dataRef),
+ llvm::omp::Clause::OMPC_depend);
+ }
}
}
}
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 33936ba4c2b34f..1c3359cc9199d1 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -25,6 +25,7 @@
#include <map>
#include <sstream>
+#include "llvm/Support/Signals.h"
template <typename T>
static Fortran::semantics::Scope *GetScope(
Fortran::semantics::SemanticsContext &context, const T &x) {
@@ -435,6 +436,19 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
bool Pre(const parser::OpenMPAllocatorsConstruct &);
void Post(const parser::OpenMPAllocatorsConstruct &);
+ void Post(const parser::OmpObjectList &x) {
+ // The objects from OMP clauses should have already been resolved,
+ // except common blocks (the ResolveNamesVisitor does not visit
+ // parser::Name, those are dealt with as members of other structures).
+ // Iterate over elements of x, and resolve any common blocks that
+ // are still unresolved.
+ for (const parser::OmpObject &obj : x.v) {
+ if (auto *name{std::get_if<parser::Name>(&obj.u)}) {
+ Resolve(*name, currScope().MakeCommonBlock(name->source));
+ }
+ }
+ }
+
// 2.15.3 Data-Sharing Attribute Clauses
void Post(const parser::OmpDefaultClause &);
bool Pre(const parser::OmpClause::Shared &x) {
@@ -531,16 +545,9 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
return false;
}
- bool Pre(const parser::OmpDependClause &x) {
- if (const auto *dependSink{
- std::get_if<parser::OmpDependClause::Sink>(&x.u)}) {
- const auto &dependSinkVec{dependSink->v};
- for (const auto &dependSinkElement : dependSinkVec) {
- const auto &name{std::get<parser::Name>(dependSinkElement.t)};
- ResolveName(&name);
- }
- }
- return false;
+ void Post(const parser::OmpDependSinkVec &x) {
+ const auto &name{std::get<parser::Name>(x.t)};
+ ResolveName(&name);
}
bool Pre(const parser::OmpClause::UseDevicePtr &x) {
diff --git a/flang/test/Semantics/OpenMP/depend04.f90 b/flang/test/Semantics/OpenMP/depend04.f90
new file mode 100644
index 00000000000000..8bdddb017d2c9d
--- /dev/null
+++ b/flang/test/Semantics/OpenMP/depend04.f90
@@ -0,0 +1,10 @@
+!RUN: %python %S/../test_errors.py %s %flang -fopenmp -fopenmp-version=50
+
+subroutine f00
+ integer :: x
+ common /cc/ x
+!ERROR: Common block name ('cc') cannot appear in a DEPEND clause
+ !$omp task depend(in: /cc/)
+ x = 0
+ !$omp end task
+end
diff --git a/llvm/include/llvm/Frontend/OpenMP/ClauseT.h b/llvm/include/llvm/Frontend/OpenMP/ClauseT.h
index ac34ddafc5e726..2b8e56ed246e59 100644
--- a/llvm/include/llvm/Frontend/OpenMP/ClauseT.h
+++ b/llvm/include/llvm/Frontend/OpenMP/ClauseT.h
@@ -503,7 +503,7 @@ struct DependT {
using LocatorList = ObjectListT<I, E>;
using TaskDependenceType = tomp::type::TaskDependenceType;
- struct WithLocators { // Modern form
+ struct DepType { // The form with task dependence type.
using TupleTrait = std::true_type;
// Empty LocatorList means "omp_all_memory".
std::tuple<TaskDependenceType, OPT(Iterator), LocatorList> t;
@@ -511,7 +511,7 @@ struct DependT {
using Doacross = DoacrossT<T, I, E>;
using UnionTrait = std::true_type;
- std::variant<Doacross, WithLocators> u; // Doacross form is legacy
+ std::variant<Doacross, DepType> u; // Doacross form is legacy
};
// V5.2: [3.5] `destroy` clause
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Background: The OpenMP spec states that a common block name cannot appear in the locator list in the DEPEND clause, so I wanted to emit a diagnostic for this. That wasn't hard, but I was seeing an additional unexpected error about an unresolved symbol in the common block. I tracked it down to the symbol resolution never visiting common blocks in OmpObjectList. The reason it happened is that [1] Named common blocks are valid list items. There are some clauses that disallow it. I don't think we handled all of that correctly in all cases, and this should get us closer to that. |
|
Ping. This is the beginning of the PR chain for changes related to DEPEND. |
Co-authored-by: Kiran Chandramohan <[email protected]>
This reverts commit 6939916. This causes a failure in depend04.f90.
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
Parse the locator list in OmpDependClause as an OmpObjectList (instead of a list of Designators). When a common block appears in the locator list, show an informative message. Implement resolving symbols in DependSinkVec in a dedicated visitor instead of having a visitor for OmpDependClause. Resolve unresolved names common blocks in OmpObjectList. Minor changes to the code organization: - rename OmpDependenceType to OmpTaskDependenceType (to follow 5.2 terminology), - rename Depend::WithLocators to Depend::DepType, - add comments with more detailed spec references to parse-tree.h. --------- Co-authored-by: Kiran Chandramohan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Parse the locator list in OmpDependClause as an OmpObjectList (instead of a list of Designators). When a common block appears in the locator list, show an informative message.
Implement resolving symbols in DependSinkVec in a dedicated visitor instead of having a visitor for OmpDependClause.
Resolve unresolved names common blocks in OmpObjectList.
Minor changes to the code organization: