[flang][OpenMP] Implement flexible OpenMP clause representation #81621
Conversation
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-flang-openmp Author: Krzysztof Parzyszek (kparzysz) ChangesThe new set of classes representing OpenMP classes mimics the contents of parser::OmpClause, but differs in a few aspects:
The class This patch only introduces the new classes, they are not yet used anywhere. [Clause representation 1/6] Patch is 35.74 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81621.diff 1 Files Affected:
diff --git a/flang/lib/Lower/OpenMP.cpp b/flang/lib/Lower/OpenMP.cpp
index 06850bebd7d05a..24bef1d999548b 100644
--- a/flang/lib/Lower/OpenMP.cpp
+++ b/flang/lib/Lower/OpenMP.cpp
@@ -142,6 +142,1121 @@ static void genNestedEvaluations(Fortran::lower::AbstractConverter &converter,
converter.genEval(e);
}
+//===----------------------------------------------------------------------===//
+// Clauses
+//===----------------------------------------------------------------------===//
+
+namespace detail {
+template <typename C> //
+llvm::omp::Clause getClauseIdForClass(C &&) {
+ using namespace Fortran;
+ using A = llvm::remove_cvref_t<C>; // A is referenced in OMP.inc
+ // The code included below contains a sequence of checks like the following
+ // for each OpenMP clause
+ // if constexpr (std::is_same_v<A, parser::OmpClause::AcqRel>)
+ // return llvm::omp::Clause::OMPC_acq_rel;
+ // [...]
+#define GEN_FLANG_CLAUSE_PARSER_KIND_MAP
+#include "llvm/Frontend/OpenMP/OMP.inc"
+}
+} // namespace detail
+
+static llvm::omp::Clause getClauseId(const Fortran::parser::OmpClause &clause) {
+ return std::visit([](auto &&s) { return detail::getClauseIdForClass(s); },
+ clause.u);
+}
+
+namespace omp {
+using namespace Fortran;
+using SomeType = evaluate::SomeType;
+using SomeExpr = semantics::SomeExpr;
+using MaybeExpr = semantics::MaybeExpr;
+
+template <typename T> using List = std::vector<T>;
+
+struct SymDsgExtractor {
+ using SymDsg = std::tuple<semantics::Symbol *, MaybeExpr>;
+
+ template <typename T> //
+ static T &&AsRvalueRef(T &&t) {
+ return std::move(t);
+ }
+ template <typename T> //
+ static T AsRvalueRef(const T &t) {
+ return t;
+ }
+
+ template <typename T> //
+ static SymDsg visit(T &&) {
+ // Use this to see missing overloads:
+ // llvm::errs() << "NULL: " << __PRETTY_FUNCTION__ << '\n';
+ return SymDsg{};
+ }
+
+ template <typename T> //
+ static SymDsg visit(const evaluate::Designator<T> &e) {
+ // Symbols cannot be created after semantic checks, so all symbol
+ // pointers that are non-null must point to one of those pre-existing
+ // objects. Throughout the code, symbols are often pointed to by
+ // non-const pointers, so there is no harm in casting the constness
+ // away.
+ return std::make_tuple(const_cast<semantics::Symbol *>(e.GetLastSymbol()),
+ evaluate::AsGenericExpr(AsRvalueRef(e)));
+ }
+
+ static SymDsg visit(const evaluate::ProcedureDesignator &e) {
+ // See comment above regarding const_cast.
+ return std::make_tuple(const_cast<semantics::Symbol *>(e.GetSymbol()),
+ std::nullopt);
+ }
+
+ template <typename T> //
+ static SymDsg visit(const evaluate::Expr<T> &e) {
+ return std::visit([](auto &&s) { return visit(s); }, e.u);
+ }
+
+ static bool verify(const SymDsg &sd) {
+ const semantics::Symbol *symbol = std::get<0>(sd);
+ assert(symbol && "Expecting Symbol");
+ auto &maybeDsg = std::get<1>(sd);
+ if (!maybeDsg)
+ return true;
+ std::optional<evaluate::DataRef> maybeRef =
+ evaluate::ExtractDataRef(*maybeDsg);
+ if (maybeRef) {
+ assert(&maybeRef->GetLastSymbol() == symbol &&
+ "Designator not for symbol");
+ return true;
+ }
+
+ // This could still be a Substring or ComplexPart, but at least Substring
+ // is not allowed in OpenMP.
+ maybeDsg->dump();
+ llvm_unreachable("Expecting DataRef");
+ }
+};
+
+SymDsgExtractor::SymDsg getSymbolAndDesignator(const MaybeExpr &expr) {
+ if (!expr)
+ return SymDsgExtractor::SymDsg{};
+ return std::visit([](auto &&s) { return SymDsgExtractor::visit(s); },
+ expr->u);
+}
+
+struct Object {
+ semantics::Symbol *sym; // symbol
+ MaybeExpr dsg; // designator ending with symbol
+};
+
+using ObjectList = List<Object>;
+
+Object makeObject(const parser::OmpObject &object,
+ semantics::SemanticsContext &semaCtx) {
+ // If object is a common block, expression analyzer won't be able to
+ // do anything.
+ if (const auto *name = std::get_if<parser::Name>(&object.u)) {
+ assert(name->symbol && "Expecting Symbol");
+ return Object{name->symbol, std::nullopt};
+ }
+ evaluate::ExpressionAnalyzer ea{semaCtx};
+ SymDsgExtractor::SymDsg sd = std::visit(
+ [&](auto &&s) { return getSymbolAndDesignator(ea.Analyze(s)); },
+ object.u);
+ SymDsgExtractor::verify(sd);
+ return Object{std::get<0>(sd), std::move(std::get<1>(sd))};
+}
+
+Object makeObject(const parser::Name &name,
+ semantics::SemanticsContext &semaCtx) {
+ assert(name.symbol && "Expecting Symbol");
+ return Object{name.symbol, std::nullopt};
+}
+
+Object makeObject(const parser::Designator &dsg,
+ semantics::SemanticsContext &semaCtx) {
+ evaluate::ExpressionAnalyzer ea{semaCtx};
+ SymDsgExtractor::SymDsg sd = getSymbolAndDesignator(ea.Analyze(dsg));
+ SymDsgExtractor::verify(sd);
+ return Object{std::get<0>(sd), std::move(std::get<1>(sd))};
+}
+
+Object makeObject(const parser::StructureComponent &comp,
+ semantics::SemanticsContext &semaCtx) {
+ evaluate::ExpressionAnalyzer ea{semaCtx};
+ SymDsgExtractor::SymDsg sd = getSymbolAndDesignator(ea.Analyze(comp));
+ SymDsgExtractor::verify(sd);
+ return Object{std::get<0>(sd), std::move(std::get<1>(sd))};
+}
+
+auto makeObject(semantics::SemanticsContext &semaCtx) {
+ return [&](auto &&s) { return makeObject(s, semaCtx); };
+}
+
+template <typename T>
+SomeExpr makeExpr(T &&inp, semantics::SemanticsContext &semaCtx) {
+ auto maybeExpr = evaluate::ExpressionAnalyzer(semaCtx).Analyze(inp);
+ assert(maybeExpr);
+ return std::move(*maybeExpr);
+}
+
+auto makeExpr(semantics::SemanticsContext &semaCtx) {
+ return [&](auto &&s) { return makeExpr(s, semaCtx); };
+}
+
+template <typename C, typename F,
+ typename E = typename llvm::remove_cvref_t<C>::value_type,
+ typename R = std::invoke_result_t<F, E>>
+List<R> makeList(C &&container, F &&func) {
+ List<R> v;
+ llvm::transform(container, std::back_inserter(v), func);
+ return v;
+}
+
+ObjectList makeList(const parser::OmpObjectList &objects,
+ semantics::SemanticsContext &semaCtx) {
+ return makeList(objects.v, makeObject(semaCtx));
+}
+
+template <typename U, typename T> //
+U enum_cast(T t) {
+ using BareT = llvm::remove_cvref_t<T>;
+ using BareU = llvm::remove_cvref_t<U>;
+ static_assert(std::is_enum_v<BareT> && std::is_enum_v<BareU>);
+
+ return U{static_cast<std::underlying_type_t<BareT>>(t)};
+}
+
+template <typename F, typename T, typename U = std::invoke_result_t<F, T>>
+std::optional<U> maybeApply(F &&func, const std::optional<T> &inp) {
+ if (!inp)
+ return std::nullopt;
+ return std::move(func(*inp));
+}
+
+namespace clause {
+#ifdef EMPTY_CLASS
+#undef EMPTY_CLASS
+#endif
+#define EMPTY_CLASS(cls) \
+ struct cls { \
+ using EmptyTrait = std::true_type; \
+ }; \
+ cls make(const parser::OmpClause::cls &, semantics::SemanticsContext &) { \
+ return cls{}; \
+ }
+
+#ifdef WRAPPER_CLASS
+#undef WRAPPER_CLASS
+#endif
+#define WRAPPER_CLASS(cls, content) // Nothing
+#define GEN_FLANG_CLAUSE_PARSER_CLASSES
+#include "llvm/Frontend/OpenMP/OMP.inc"
+#undef EMPTY_CLASS
+
+// Helper objects
+
+struct DefinedOperator {
+ struct DefinedOpName {
+ using WrapperTrait = std::true_type;
+ Object v;
+ };
+ ENUM_CLASS(IntrinsicOperator, Power, Multiply, Divide, Add, Subtract, Concat,
+ LT, LE, EQ, NE, GE, GT, NOT, AND, OR, EQV, NEQV)
+ using UnionTrait = std::true_type;
+ std::variant<DefinedOpName, IntrinsicOperator> u;
+};
+
+DefinedOperator makeDefOp(const parser::DefinedOperator &inp,
+ semantics::SemanticsContext &semaCtx) {
+ return DefinedOperator{
+ std::visit(common::visitors{
+ [&](const parser::DefinedOpName &s) {
+ return DefinedOperator{DefinedOperator::DefinedOpName{
+ makeObject(s.v, semaCtx)}};
+ },
+ [&](const parser::DefinedOperator::IntrinsicOperator &s) {
+ return DefinedOperator{
+ enum_cast<DefinedOperator::IntrinsicOperator>(s)};
+ },
+ },
+ inp.u),
+ };
+}
+
+struct ProcedureDesignator {
+ using WrapperTrait = std::true_type;
+ Object v;
+};
+
+ProcedureDesignator makeProcDsg(const parser::ProcedureDesignator &inp,
+ semantics::SemanticsContext &semaCtx) {
+ return ProcedureDesignator{std::visit(
+ common::visitors{
+ [&](const parser::Name &t) { return makeObject(t, semaCtx); },
+ [&](const parser::ProcComponentRef &t) {
+ return makeObject(t.v.thing, semaCtx);
+ },
+ },
+ inp.u)};
+}
+
+struct ReductionOperator {
+ using UnionTrait = std::true_type;
+ std::variant<DefinedOperator, ProcedureDesignator> u;
+};
+
+ReductionOperator makeRedOp(const parser::OmpReductionOperator &inp,
+ semantics::SemanticsContext &semaCtx) {
+ return std::visit(common::visitors{
+ [&](const parser::DefinedOperator &s) {
+ return ReductionOperator{makeDefOp(s, semaCtx)};
+ },
+ [&](const parser::ProcedureDesignator &s) {
+ return ReductionOperator{makeProcDsg(s, semaCtx)};
+ },
+ },
+ inp.u);
+}
+
+// Actual clauses. Each T (where OmpClause::T exists) has its "make".
+
+struct Aligned {
+ using TupleTrait = std::true_type;
+ std::tuple<ObjectList, MaybeExpr> t;
+};
+
+Aligned make(const parser::OmpClause::Aligned &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpAlignedClause
+ auto &t0 = std::get<parser::OmpObjectList>(inp.v.t);
+ auto &t1 = std::get<std::optional<parser::ScalarIntConstantExpr>>(inp.v.t);
+
+ return Aligned{{
+ makeList(t0, semaCtx),
+ maybeApply(makeExpr(semaCtx), t1),
+ }};
+}
+
+struct Allocate {
+ struct Modifier {
+ struct Allocator {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+ };
+ struct Align {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+ };
+ struct ComplexModifier {
+ using TupleTrait = std::true_type;
+ std::tuple<Allocator, Align> t;
+ };
+ using UnionTrait = std::true_type;
+ std::variant<Allocator, ComplexModifier, Align> u;
+ };
+ using TupleTrait = std::true_type;
+ std::tuple<std::optional<Modifier>, ObjectList> t;
+};
+
+Allocate make(const parser::OmpClause::Allocate &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpAllocateClause
+ using wrapped = parser::OmpAllocateClause;
+ auto &t0 = std::get<std::optional<wrapped::AllocateModifier>>(inp.v.t);
+ auto &t1 = std::get<parser::OmpObjectList>(inp.v.t);
+
+ auto convert = [&](auto &&s) -> Allocate::Modifier {
+ using Modifier = Allocate::Modifier;
+ using Allocator = Modifier::Allocator;
+ using Align = Modifier::Align;
+ using ComplexModifier = Modifier::ComplexModifier;
+
+ return Modifier{
+ std::visit(
+ common::visitors{
+ [&](const wrapped::AllocateModifier::Allocator &v) {
+ return Modifier{Allocator{makeExpr(v.v, semaCtx)}};
+ },
+ [&](const wrapped::AllocateModifier::ComplexModifier &v) {
+ auto &s0 =
+ std::get<wrapped::AllocateModifier::Allocator>(v.t);
+ auto &s1 = std::get<wrapped::AllocateModifier::Align>(v.t);
+ return Modifier{ComplexModifier{{
+ Allocator{makeExpr(s0.v, semaCtx)},
+ Align{makeExpr(s1.v, semaCtx)},
+ }}};
+ },
+ [&](const wrapped::AllocateModifier::Align &v) {
+ return Modifier{Align{makeExpr(v.v, semaCtx)}};
+ },
+ },
+ s.u),
+ };
+ };
+
+ return Allocate{{maybeApply(convert, t0), makeList(t1, semaCtx)}};
+}
+
+struct Allocator {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+};
+
+Allocator make(const parser::OmpClause::Allocator &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::ScalarIntExpr
+ return Allocator{makeExpr(inp.v, semaCtx)};
+}
+
+struct AtomicDefaultMemOrder {
+ using WrapperTrait = std::true_type;
+ common::OmpAtomicDefaultMemOrderType v;
+};
+
+AtomicDefaultMemOrder make(const parser::OmpClause::AtomicDefaultMemOrder &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpAtomicDefaultMemOrderClause
+ return AtomicDefaultMemOrder{inp.v.v};
+}
+
+struct Collapse {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+};
+
+Collapse make(const parser::OmpClause::Collapse &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::ScalarIntConstantExpr
+ return Collapse{makeExpr(inp.v, semaCtx)};
+}
+
+struct Copyin {
+ using WrapperTrait = std::true_type;
+ ObjectList v;
+};
+
+Copyin make(const parser::OmpClause::Copyin &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpObjectList
+ return Copyin{makeList(inp.v, semaCtx)};
+}
+
+struct Copyprivate {
+ using WrapperTrait = std::true_type;
+ ObjectList v;
+};
+
+Copyprivate make(const parser::OmpClause::Copyprivate &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpObjectList
+ return Copyprivate{makeList(inp.v, semaCtx)};
+}
+
+struct Defaultmap {
+ ENUM_CLASS(ImplicitBehavior, Alloc, To, From, Tofrom, Firstprivate, None,
+ Default)
+ ENUM_CLASS(VariableCategory, Scalar, Aggregate, Allocatable, Pointer)
+ using TupleTrait = std::true_type;
+ std::tuple<ImplicitBehavior, std::optional<VariableCategory>> t;
+};
+
+Defaultmap make(const parser::OmpClause::Defaultmap &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpDefaultmapClause
+ using wrapped = parser::OmpDefaultmapClause;
+
+ auto convert = [](auto &&s) -> Defaultmap::VariableCategory {
+ return enum_cast<Defaultmap::VariableCategory>(s);
+ };
+ auto &t0 = std::get<wrapped::ImplicitBehavior>(inp.v.t);
+ auto &t1 = std::get<std::optional<wrapped::VariableCategory>>(inp.v.t);
+ auto v0 = enum_cast<Defaultmap::ImplicitBehavior>(t0);
+ return Defaultmap{{v0, maybeApply(convert, t1)}};
+}
+
+struct Default {
+ ENUM_CLASS(Type, Private, Firstprivate, Shared, None)
+ using WrapperTrait = std::true_type;
+ Type v;
+};
+
+Default make(const parser::OmpClause::Default &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpDefaultClause
+ return Default{enum_cast<Default::Type>(inp.v.v)};
+}
+
+struct Depend {
+ struct Source {
+ using EmptyTrait = std::true_type;
+ };
+ struct Sink {
+ using Length = std::tuple<DefinedOperator, SomeExpr>;
+ using Vec = std::tuple<Object, std::optional<Length>>;
+ using WrapperTrait = std::true_type;
+ List<Vec> v;
+ };
+ ENUM_CLASS(Type, In, Out, Inout, Source, Sink)
+ struct InOut {
+ using TupleTrait = std::true_type;
+ std::tuple<Type, ObjectList> t;
+ };
+ using UnionTrait = std::true_type;
+ std::variant<Source, Sink, InOut> u;
+};
+
+Depend make(const parser::OmpClause::Depend &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpDependClause
+ using wrapped = parser::OmpDependClause;
+
+ return std::visit(
+ common::visitors{
+ [&](const wrapped::Source &s) { return Depend{Depend::Source{}}; },
+ [&](const wrapped::Sink &s) {
+ auto convert = [&](const parser::OmpDependSinkVec &v) {
+ auto &t0 = std::get<parser::Name>(v.t);
+ auto &t1 =
+ std::get<std::optional<parser::OmpDependSinkVecLength>>(v.t);
+ auto convert1 = [&](const parser::OmpDependSinkVecLength &u) {
+ auto &s0 = std::get<parser::DefinedOperator>(u.t);
+ auto &s1 = std::get<parser::ScalarIntConstantExpr>(u.t);
+ return Depend::Sink::Length{makeDefOp(s0, semaCtx),
+ makeExpr(s1, semaCtx)};
+ };
+ return Depend::Sink::Vec{makeObject(t0, semaCtx),
+ maybeApply(convert1, t1)};
+ };
+ return Depend{Depend::Sink{makeList(s.v, convert)}};
+ },
+ [&](const wrapped::InOut &s) {
+ auto &t0 = std::get<parser::OmpDependenceType>(s.t);
+ auto &t1 = std::get<std::list<parser::Designator>>(s.t);
+ auto convert = [&](const parser::Designator &t) {
+ return makeObject(t, semaCtx);
+ };
+ return Depend{Depend::InOut{
+ {enum_cast<Depend::Type>(t0.v), makeList(t1, convert)}}};
+ },
+ },
+ inp.v.u);
+}
+
+struct Device {
+ ENUM_CLASS(DeviceModifier, Ancestor, Device_Num)
+ using TupleTrait = std::true_type;
+ std::tuple<std::optional<DeviceModifier>, SomeExpr> t;
+};
+
+Device make(const parser::OmpClause::Device &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpDeviceClause
+ using wrapped = parser::OmpDeviceClause;
+
+ auto convert = [](auto &&s) -> Device::DeviceModifier {
+ return enum_cast<Device::DeviceModifier>(s);
+ };
+ auto &t0 = std::get<std::optional<wrapped::DeviceModifier>>(inp.v.t);
+ auto &t1 = std::get<parser::ScalarIntExpr>(inp.v.t);
+ return Device{{maybeApply(convert, t0), makeExpr(t1, semaCtx)}};
+}
+
+struct DeviceType {
+ ENUM_CLASS(Type, Any, Host, Nohost)
+ using WrapperTrait = std::true_type;
+ Type v;
+};
+
+DeviceType make(const parser::OmpClause::DeviceType &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpDeviceTypeClause
+ return DeviceType{enum_cast<DeviceType::Type>(inp.v.v)};
+}
+
+struct DistSchedule {
+ using WrapperTrait = std::true_type;
+ MaybeExpr v;
+};
+
+DistSchedule make(const parser::OmpClause::DistSchedule &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> std::optional<parser::ScalarIntExpr>
+ return DistSchedule{maybeApply(makeExpr(semaCtx), inp.v)};
+}
+
+struct Enter {
+ using WrapperTrait = std::true_type;
+ ObjectList v;
+};
+
+Enter make(const parser::OmpClause::Enter &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpObjectList
+ return Enter{makeList(inp.v, semaCtx)};
+}
+
+struct Filter {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+};
+
+Filter make(const parser::OmpClause::Filter &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::ScalarIntExpr
+ return Filter{makeExpr(inp.v, semaCtx)};
+}
+
+struct Final {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+};
+
+Final make(const parser::OmpClause::Final &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::ScalarLogicalExpr
+ return Final{makeExpr(inp.v, semaCtx)};
+}
+
+struct Firstprivate {
+ using WrapperTrait = std::true_type;
+ ObjectList v;
+};
+
+Firstprivate make(const parser::OmpClause::Firstprivate &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpObjectList
+ return Firstprivate{makeList(inp.v, semaCtx)};
+}
+
+struct From {
+ using WrapperTrait = std::true_type;
+ ObjectList v;
+};
+
+From make(const parser::OmpClause::From &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::OmpObjectList
+ return From{makeList(inp.v, semaCtx)};
+}
+
+struct Grainsize {
+ using WrapperTrait = std::true_type;
+ SomeExpr v;
+};
+
+Grainsize make(const parser::OmpClause::Grainsize &inp,
+ semantics::SemanticsContext &semaCtx) {
+ // inp.v -> parser::ScalarIntExpr
+ return Grainsize{makeExpr(inp.v, semaCtx)};...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
kparzysz
force-pushed
the
users/kparzysz/spr/b01-clauses
branch
from
5e67fca
to
f0bcbbf
Compare
|
Added reuse of enum definitions from Fortran::parser, as suggested by @skatrak in the community call. |
There was a problem hiding this comment.
Thank you Krzysztof for this work. I added a few comments inline, but they are rather minor nits and suggestions.
I see that this approach requires representing the complete list of clauses, and that's the reason for the size of this patch. However, these are very much tied to the flang PFT, so I wonder whether it would be feasible to get this to generically work for clang as well and make it part of llvm/Frontend/OpenMP, as it was discussed in yesterday's community call.
For example, the reduction clause is defined here and in the PFT as a ReductionOperator and an ObjectList, and the ReductionOperator is made of a DefinedOperator or a ProcedureDesignator. In clang, the corresponding structure inherits from OMPVarListClause and contains a DeclarationNameInfo, among others. So I can foresee some issues creating make() functions for these structures based on the clang AST, since representations are quite different. Also, the various traits are only used by flang's PFT visitors and the main Object class here uses Fortran::semantics types, which I'm not sure whether it could be easily templated to contain either flang or clang symbols/expressions.
So I'm not sure whether the list of structures here could be made somewhat more language agnostic and based off of the spec rather than flang's PFT if there's a chance that this approach could be shared by clang and flang.
flang/lib/Lower/OpenMP.cpp
Outdated
| #define WRAPPER_CLASS(cls, content) // Nothing | ||
| #define GEN_FLANG_CLAUSE_PARSER_CLASSES | ||
| #include "llvm/Frontend/OpenMP/OMP.inc" | ||
| #undef EMPTY_CLASS |
There was a problem hiding this comment.
Is there a reason to #undef EMPTY_CLASS but not WRAPPER_CLASS here?
There was a problem hiding this comment.
It's an omission. Both of these macros are "global" in the sense that they are defined in some header and used elsewhere. Since I redefine them, I don't want my redefinitions to leak out and cause confusion.
The TableGen-generated code should not be using these macros directly. It's an easy fix, and would solve this issue.
There was a problem hiding this comment.
The TableGen-generated code should not be using these macros directly.
Why?
GEN_FLANG_CLAUSE_PARSER_CLASSES is generated for the parse-tree and just not meant to be included here.
There was a problem hiding this comment.
Because it forces a specific interface between the autogenerated .inc file and its uses in the C++ sources. TableGen-generated files that are "enumerative" in nature shouldn't need to be aware of the context where they're used. Things that have a single application today may have multiple tomorrow.
There was a problem hiding this comment.
Yes and this is indeed induced by the name of the including def. There is no harm to have another entry point in the DirectiveEmitter that produces a more generic list that is not meant for the parser nodes. This would also remove the need to use macro that are used specifically in flang parse-tree.
There was a problem hiding this comment.
What TG generates here is a list of pairs (class-name, contents). In some cases we may need both, in other cases we may only need the class name. Conceptually, both are different pieces of the same root information. We can indeed generate more information in TG, but we wouldn't need to if the information was done generically from the start.
flang/lib/Lower/OpenMP.cpp
Outdated
| #define WRAPPER_CLASS(cls, content) // Nothing | ||
| #define GEN_FLANG_CLAUSE_PARSER_CLASSES | ||
| #include "llvm/Frontend/OpenMP/OMP.inc" | ||
| #undef EMPTY_CLASS |
There was a problem hiding this comment.
It's an omission. Both of these macros are "global" in the sense that they are defined in some header and used elsewhere. Since I redefine them, I don't want my redefinitions to leak out and cause confusion.
The TableGen-generated code should not be using these macros directly. It's an easy fix, and would solve this issue.
This is a very good point. Yes, that would be better, and that's the next step here, but it would be good to have something working before that, so the changes are more incremental. |
kparzysz
force-pushed
the
users/kparzysz/spr/b01-clauses
branch
from
668bd37
to
e37dd7d
Compare
I was working on this today, and it would introduce changes that could obfuscate the picture, so I decided to go with the current definitions for this PR. It's in the works, though. |
Introduce a set of generic classes (templates) that represent OpenMP clauses in a language-agnostic manner. OpenMP clauses can contain expressions and data objects and the exact representation of each depends on the source language of the compiled program. To deal with this, the templates depend on two type parameters: - IdType: type that represent object's identity (in a way that satisfied OpenMP requirements), and - ExprType: type that can represent numeric values, as well as data references (e.g. x.y[1].z[2]). In addition to that, implement code instantiating these templates from flang's AST. This patch only introduces the new classes, they are not yet used anywhere.
kparzysz
force-pushed
the
users/kparzysz/spr/b01-clauses
branch
from
e37dd7d
to
e16585a
Compare
|
Ping. Are there still concerns about this? |
|
Could you expand the summary to briefly mention the contents of the patch? |
|
Thanks Kiran. I expanded the PR description with additional information. The creation of omp::Clause from parser::OmpClause can be done with a single function call: so the complexity of creating one is relatively small. If we created them during PTF creation, we'd need to store them somewhere (in OmpClause itself?). Replacing OmpClause with omp::Clause would require potentially invasive changes in the parser/semantics analyzer, plus it would deviate from the representations of other PFT nodes (specifically SomeExpr/MaybeExpr is not explicitly used in PFT, except in parser::Expr). I'm not sure if this answers your question though. Let me know if you'd like more information. |
| template <typename Id, typename Expr> | ||
| struct ObjectT; | ||
|
|
||
| template <typename I, typename E> |
There was a problem hiding this comment.
Nit: Could you spell out Id and Expr everywhere rather than just for ObjectT?
| template <typename I, typename E> | |
| template <typename Id, typename Expr> |
There was a problem hiding this comment.
I'd prefer not---this would create too much clutter. I specifically avoided spelling it out everywhere, except where the parameters are introduced.
There was a problem hiding this comment.
We're going to have to agree to disagree on this one, but I won't block approving this patch based on this minor thing which is largely just a matter of preference 😄. Maybe someone else can chime in to make a decision.
There was a problem hiding this comment.
Thank you Krzysztof for all the work (and for your patience with my nits!). I think this is good to go, but since it's a big change and there are a few other PRs coming right after, I'd suggest waiting for @kiranchandramohan's approval as well before merging.
|
Windows build: |
|
Hi @kiranchandramohan, are you ok with merging this? |
Yes. Nice work! And thanks @skatrak for the detailed review. |
|
Again seeing a bunch of |
|
@kparzysz Peter Klausler mentioned that the build is broken. Could you have a look? |
The new set of classes representing OpenMP classes mimics the contents of parser::OmpClause, but differs in a few aspects:
This set of classes is geared towards a language-agnostic representation of OpenMP clauses. While the class members are still based on flang's
parser::OmpClause, the classes themselves are actually C++ templates parameterized with types essential to represent necessary information. The two parameters areIdType: the type that can represent object's identity (for flang it will besemantics::Symbol *),ExprType: the type that can represent expressions, arithmetic and object references (semantics::MaybeExprin flang).The templates are defined in a namespace
tomp(to distinguish it fromomp).This patch introduces file "Clauses.cpp", which contains instantiations of all of the templates for flang. The instantiations are members of namespace
omp, and include an all-encompassing variant classomp::Clause, which is the analog ofparser::OmpClause.The class
OmpObjectis represented byomp::Object, which contains the symbol associated with the object, andsemantics::MaybeExprrepresenting the designator for the symbol reference. For each specific clause in the variantparser::OmpClause, there exists amakefunction that will generate the correspondingomp::Clausefrom it. The intent was to use the make functions as variant visitors. The creation of a clause instance would then follow the pattern:If a new clause
foois added in the future, then:tomp::FooTrepresenting the clause needs to be added to ClauseT.h,using Foo = tomp::FooT<...>,parser::OmpClause::Foo.This patch only introduces the new classes, they are not yet used anywhere.
[Clause representation 1/6]