Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Frontend][OpenMP] Refactor getLeafConstructs, add getCompoundConstruct #87247

Merged
merged 19 commits into from
Apr 22, 2024

Conversation

kparzysz
Copy link
Contributor

@kparzysz kparzysz commented Apr 1, 2024

Emit a special leaf construct table in DirectiveEmitter.cpp, which will allow both decomposition of a construct into leafs, and composition of constituent constructs into a single compound construct (if possible). The function getLeafConstructs is no longer auto-generated, but implemented in OMP.cpp.

The table contains a row for each directive, and each row has the following format
dir_id, num_leafs, leaf1, leaf2, ..., leafN, -1, ...
The rows are sorted lexicographically with respect to the leaf constructs. This allows a binary search for the row corresponding to the given list of leafs.

There is an auxiliary table that for each directive contains the index of the row corresponding to that directive.

Looking up leaf constructs for a directive dir_id is of constant time, and and consists of two lookups: LeafTable[Auxiliary[dir_id]].
Finding a compound directive given the set of leafs is of time O(logn), and is roughly represented by
row = binary_search(LeafTable); return row[0].

The functions getLeafConstructs and getCompoundConstruct use these lookup methods internally.

skatrak and others added 8 commits March 27, 2024 12:15
This patch introduces a set of composable structures grouping the MLIR operands
associated to each OpenMP clause. This makes it easier to keep the MLIR
representation for the same clause consistent throughout all operations that
accept it.

The relevant clause operand structures are grouped into per-operation
structures using a mixin pattern and used to define new operation constructors.
These constructors can be used to avoid having to get the order of a possibly
large list of operands right.

Missing clauses are documented as TODOs, as well as operands which are part of
the relevant operation's operand structure but cannot be attached to the
associated operation yet, due to missing op arguments to its MLIR definition.

A follow-up patch will update Flang lowering to make use of these structures,
simplifying the passing of information from clause processing to operation-
generating functions and also simplifying the creation of operations through
the use of the new operation constructors.
This patch updates Flang lowering to use the new set of OpenMP clause operand
structures and their groupings into directive-specific sets of clause operands.

It simplifies the passing of information from the clause processor and the
creation of operations.

The `DataSharingProcessor` is slightly modified to not hold delayed
privatization state. Instead, optional arguments are added to `processStep1`
which are only passed when delayed privatization is used. This enables using
the clause operand structure for `private` and removes the need for the ad-hoc
`DelayedPrivatizationInfo` structure.

The processing of the `schedule` clause is updated to process the `chunk`
modifier rather than requiring two separate calls to the `ClauseProcessor`.

Lowering of a block-associated `ordered` construct is updated to emit a TODO
error if the `simd` clause is specified, since it is not currently supported by
the `ClauseProcessor` or later compilation stages.

Removed processing of `schedule` from `omp.simdloop`, as it doesn't apply to
`simd` constructs.
This patch performs several cleanups with the main purpose of normalizing the
code patterns used to trigger codegen for MLIR OpenMP operations and making the
processing of clauses and constructs independent. The following changes are
made:

- Clean up unused `directive` argument to `ClauseProcessor::processMap()`.
- Move general helper functions in OpenMP.cpp to the appropriate section of the
file.
- Create `gen<OpName>Clauses()` functions containing the clause processing code
specific for the associated OpenMP construct.
- Update `gen<OpName>Op()` functions to call the corresponding
`gen<OpName>Clauses()` function.
- Sort calls to `ClauseProcessor::process<ClauseName>()` alphabetically, to
avoid inadvertently relying on some arbitrary order. Update some tests that
broke due to the order change.
- Normalize `genOMP()` functions so they all delegate the generation of MLIR to
`gen<OpName>Op()` functions following the same pattern.
- Only process `nowait` clause on `TARGET` constructs if not compiling for the
target device.

A later patch can move the calls to `gen<OpName>Clauses()` out of
`gen<OpName>Op()` functions and passing completed clause structures instead, in
preparation to supporting composite constructs. That will make it possible to
reuse clause processing for a given leaf construct when appearing alone or in a
combined or composite construct, while controlling where the associated code is
produced.
This patch simplifies the lowering from PFT to MLIR of OpenMP compound
constructs (i.e. combined and composite).

The new approach consists of iteratively processing the outermost leaf
construct of the given combined construct until it cannot be split further.
Both leaf constructs and composite ones have `gen...()` functions that are
called when appropriate.

This approach enables treating a leaf construct the same way regardless of if
it appeared as part of a combined construct, and it also enables the lowering
of composite constructs as a single unit.

Previous corner cases are now handled in a more straightforward way and
comments pointing to the relevant spec section are added. Directive sets are
also completed with missing LOOP related constructs.
This removes the last use of genOmpObectList2, which has now been removed.
@llvmbot llvmbot added flang:openmp clang:openmp OpenMP related changes to Clang labels Apr 1, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 1, 2024

@llvm/pr-subscribers-flang-openmp

Author: Krzysztof Parzyszek (kparzysz)

Changes

Emit a special leaf constuct table in DirectiveEmitter.cpp, which will allow both decomposition of a construct into leafs, and composition of constituent constructs into a single compound construct (is possible).


Full diff: https://github.com/llvm/llvm-project/pull/87247.diff

5 Files Affected:

  • (modified) llvm/include/llvm/Frontend/OpenMP/OMP.h (+7)
  • (modified) llvm/lib/Frontend/OpenMP/OMP.cpp (+63-1)
  • (modified) llvm/unittests/Frontend/CMakeLists.txt (+1)
  • (added) llvm/unittests/Frontend/OpenMPComposeTest.cpp (+40)
  • (modified) llvm/utils/TableGen/DirectiveEmitter.cpp (+124-70)
diff --git a/llvm/include/llvm/Frontend/OpenMP/OMP.h b/llvm/include/llvm/Frontend/OpenMP/OMP.h
index a85cd9d344c6d7..4ed47f15dfe59e 100644
--- a/llvm/include/llvm/Frontend/OpenMP/OMP.h
+++ b/llvm/include/llvm/Frontend/OpenMP/OMP.h
@@ -15,4 +15,11 @@
 
 #include "llvm/Frontend/OpenMP/OMP.h.inc"
 
+#include "llvm/ADT/ArrayRef.h"
+
+namespace llvm::omp {
+ArrayRef<Directive> getLeafConstructs(Directive D);
+Directive getCompoundConstruct(ArrayRef<Directive> Parts);
+} // namespace llvm::omp
+
 #endif // LLVM_FRONTEND_OPENMP_OMP_H
diff --git a/llvm/lib/Frontend/OpenMP/OMP.cpp b/llvm/lib/Frontend/OpenMP/OMP.cpp
index 4f2f95392648b3..dd99d3d074fd1e 100644
--- a/llvm/lib/Frontend/OpenMP/OMP.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMP.cpp
@@ -8,12 +8,74 @@
 
 #include "llvm/Frontend/OpenMP/OMP.h"
 
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorHandling.h"
 
+#include <algorithm>
+#include <iterator>
+#include <type_traits>
+
 using namespace llvm;
-using namespace omp;
+using namespace llvm::omp;
 
 #define GEN_DIRECTIVES_IMPL
 #include "llvm/Frontend/OpenMP/OMP.inc"
+
+namespace llvm::omp {
+ArrayRef<Directive> getLeafConstructs(Directive D) {
+  auto Idx = static_cast<int>(D);
+  if (Idx < 0 || Idx >= static_cast<int>(Directive_enumSize))
+    return {};
+  const auto *Row = LeafConstructTable[LeafConstructTableOrdering[Idx]];
+  return ArrayRef(&Row[2], &Row[2] + static_cast<int>(Row[1]));
+}
+
+Directive getCompoundConstruct(ArrayRef<Directive> Parts) {
+  if (Parts.empty())
+    return OMPD_unknown;
+
+  // Parts don't have to be leafs, so expand them into leafs first.
+  // Store the expanded leafs in the same format as rows in the leaf
+  // table (generated by tablegen).
+  SmallVector<Directive> RawLeafs(2);
+  for (Directive P : Parts) {
+    ArrayRef<Directive> Ls = getLeafConstructs(P);
+    if (!Ls.empty())
+      RawLeafs.append(Ls.begin(), Ls.end());
+    else
+      RawLeafs.push_back(P);
+  }
+
+  auto GivenLeafs{ArrayRef<Directive>(RawLeafs).drop_front(2)};
+  if (GivenLeafs.size() == 1)
+    return GivenLeafs.front();
+  RawLeafs[1] = static_cast<Directive>(GivenLeafs.size());
+
+  auto Iter = llvm::lower_bound(
+      LeafConstructTable,
+      static_cast<std::decay_t<decltype(*LeafConstructTable)>>(RawLeafs.data()),
+      [](const auto *RowA, const auto *RowB) {
+        const auto *BeginA = &RowA[2];
+        const auto *EndA = BeginA + static_cast<int>(RowA[1]);
+        const auto *BeginB = &RowB[2];
+        const auto *EndB = BeginB + static_cast<int>(RowB[1]);
+        if (BeginA == EndA && BeginB == EndB)
+          return static_cast<int>(RowA[0]) < static_cast<int>(RowB[0]);
+        return std::lexicographical_compare(BeginA, EndA, BeginB, EndB);
+      });
+
+  if (Iter == std::end(LeafConstructTable))
+    return OMPD_unknown;
+
+  // Verify that we got a match.
+  Directive Found = (*Iter)[0];
+  ArrayRef<Directive> FoundLeafs = getLeafConstructs(Found);
+  if (FoundLeafs == GivenLeafs)
+    return Found;
+  return OMPD_unknown;
+}
+} // namespace llvm::omp
diff --git a/llvm/unittests/Frontend/CMakeLists.txt b/llvm/unittests/Frontend/CMakeLists.txt
index c6f60142d6276a..ddb6a16cbb984e 100644
--- a/llvm/unittests/Frontend/CMakeLists.txt
+++ b/llvm/unittests/Frontend/CMakeLists.txt
@@ -14,6 +14,7 @@ add_llvm_unittest(LLVMFrontendTests
   OpenMPContextTest.cpp
   OpenMPIRBuilderTest.cpp
   OpenMPParsingTest.cpp
+  OpenMPComposeTest.cpp
 
   DEPENDS
   acc_gen
diff --git a/llvm/unittests/Frontend/OpenMPComposeTest.cpp b/llvm/unittests/Frontend/OpenMPComposeTest.cpp
new file mode 100644
index 00000000000000..2dc35aca8842e9
--- /dev/null
+++ b/llvm/unittests/Frontend/OpenMPComposeTest.cpp
@@ -0,0 +1,40 @@
+//===- llvm/unittests/Frontend/OpenMPComposeTest.cpp ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/Frontend/OpenMP/OMP.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace llvm::omp;
+
+TEST(Composition, GetLeafConstructs) {
+  ArrayRef<Directive> L1 = getLeafConstructs(OMPD_loop);
+  ASSERT_EQ(L1, (ArrayRef<Directive>{}));
+  ArrayRef<Directive> L2 = getLeafConstructs(OMPD_parallel_for);
+  ASSERT_EQ(L2, (ArrayRef<Directive>{OMPD_parallel, OMPD_for}));
+  ArrayRef<Directive> L3 = getLeafConstructs(OMPD_parallel_for_simd);
+  ASSERT_EQ(L3, (ArrayRef<Directive>{OMPD_parallel, OMPD_for, OMPD_simd}));
+}
+
+TEST(Composition, GetCompoundConstruct) {
+  Directive C1 = getCompoundConstruct({OMPD_target, OMPD_teams, OMPD_distribute});
+  ASSERT_EQ(C1, OMPD_target_teams_distribute);
+  Directive C2 = getCompoundConstruct({OMPD_target});
+  ASSERT_EQ(C2, OMPD_target);
+  Directive C3 = getCompoundConstruct({OMPD_target, OMPD_masked});
+  ASSERT_EQ(C3, OMPD_unknown);
+  Directive C4 = getCompoundConstruct({OMPD_target, OMPD_teams_distribute});
+  ASSERT_EQ(C4, OMPD_target_teams_distribute);
+  Directive C5 = getCompoundConstruct({OMPD_target, OMPD_teams_distribute});
+  ASSERT_EQ(C5, OMPD_target_teams_distribute);
+  Directive C6 = getCompoundConstruct({});
+  ASSERT_EQ(C6, OMPD_unknown);
+  Directive C7 = getCompoundConstruct({OMPD_parallel_for, OMPD_simd});
+  ASSERT_EQ(C7, OMPD_parallel_for_simd);
+}
diff --git a/llvm/utils/TableGen/DirectiveEmitter.cpp b/llvm/utils/TableGen/DirectiveEmitter.cpp
index e0edf1720f8ac5..34b517e816a243 100644
--- a/llvm/utils/TableGen/DirectiveEmitter.cpp
+++ b/llvm/utils/TableGen/DirectiveEmitter.cpp
@@ -20,6 +20,9 @@
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 
+#include <numeric>
+#include <vector>
+
 using namespace llvm;
 
 namespace {
@@ -39,7 +42,8 @@ class IfDefScope {
 };
 } // namespace
 
-// Generate enum class
+// Generate enum class. Entries are emitted in the order in which they appear
+// in the `Records` vector.
 static void GenerateEnumClass(const std::vector<Record *> &Records,
                               raw_ostream &OS, StringRef Enum, StringRef Prefix,
                               const DirectiveLanguage &DirLang,
@@ -175,6 +179,16 @@ bool DirectiveLanguage::HasValidityErrors() const {
   return HasDuplicateClausesInDirectives(getDirectives());
 }
 
+// Count the maximum number of leaf constituents per construct.
+static size_t GetMaxLeafCount(const DirectiveLanguage &DirLang) {
+  size_t MaxCount = 0;
+  for (Record *R : DirLang.getDirectives()) {
+    size_t Count = Directive{R}.getLeafConstructs().size();
+    MaxCount = std::max(MaxCount, Count);
+  }
+  return MaxCount;
+}
+
 // Generate the declaration section for the enumeration in the directive
 // language
 static void EmitDirectivesDecl(RecordKeeper &Records, raw_ostream &OS) {
@@ -189,6 +203,7 @@ static void EmitDirectivesDecl(RecordKeeper &Records, raw_ostream &OS) {
   if (DirLang.hasEnableBitmaskEnumInNamespace())
     OS << "#include \"llvm/ADT/BitmaskEnum.h\"\n";
 
+  OS << "#include <cstddef>\n"; // for size_t
   OS << "\n";
   OS << "namespace llvm {\n";
   OS << "class StringRef;\n";
@@ -244,7 +259,8 @@ static void EmitDirectivesDecl(RecordKeeper &Records, raw_ostream &OS) {
   OS << "bool isAllowedClauseForDirective(Directive D, "
      << "Clause C, unsigned Version);\n";
   OS << "\n";
-  OS << "llvm::ArrayRef<Directive> getLeafConstructs(Directive D);\n";
+  OS << "constexpr std::size_t getMaxLeafCount() { return "
+     << GetMaxLeafCount(DirLang) << "; }\n";
   OS << "Association getDirectiveAssociation(Directive D);\n";
   if (EnumHelperFuncs.length() > 0) {
     OS << EnumHelperFuncs;
@@ -396,6 +412,19 @@ GenerateCaseForVersionedClauses(const std::vector<Record *> &Clauses,
   }
 }
 
+static std::string GetDirectiveName(const DirectiveLanguage &DirLang,
+                                    const Record *Rec) {
+  Directive Dir{Rec};
+  return (llvm::Twine("llvm::") + DirLang.getCppNamespace() + "::" +
+          DirLang.getDirectivePrefix() + Dir.getFormattedName())
+      .str();
+}
+
+static std::string GetDirectiveType(const DirectiveLanguage &DirLang) {
+  return (llvm::Twine("llvm::") + DirLang.getCppNamespace() + "::Directive")
+      .str();
+}
+
 // Generate the isAllowedClauseForDirective function implementation.
 static void GenerateIsAllowedClause(const DirectiveLanguage &DirLang,
                                     raw_ostream &OS) {
@@ -450,77 +479,102 @@ static void GenerateIsAllowedClause(const DirectiveLanguage &DirLang,
   OS << "}\n"; // End of function isAllowedClauseForDirective
 }
 
-// Generate the getLeafConstructs function implementation.
-static void GenerateGetLeafConstructs(const DirectiveLanguage &DirLang,
-                                      raw_ostream &OS) {
-  auto getQualifiedName = [&](StringRef Formatted) -> std::string {
-    return (llvm::Twine("llvm::") + DirLang.getCppNamespace() +
-            "::Directive::" + DirLang.getDirectivePrefix() + Formatted)
-        .str();
-  };
-
-  // For each list of leaves, generate a static local object, then
-  // return a reference to that object for a given directive, e.g.
+static void EmitLeafTable(const DirectiveLanguage &DirLang, raw_ostream &OS,
+                          StringRef TableName) {
+  // The leaf constructs are emitted in a form of a 2D table, where each
+  // row corresponds to a directive (and there is a row for each directive).
   //
-  //   static ListTy leafConstructs_A_B = { A, B };
-  //   static ListTy leafConstructs_C_D_E = { C, D, E };
-  //   switch (Dir) {
-  //     case A_B:
-  //       return leafConstructs_A_B;
-  //     case C_D_E:
-  //       return leafConstructs_C_D_E;
-  //   }
-
-  // Map from a record that defines a directive to the name of the
-  // local object with the list of its leaves.
-  DenseMap<Record *, std::string> ListNames;
-
-  std::string DirectiveTypeName =
-      std::string("llvm::") + DirLang.getCppNamespace().str() + "::Directive";
-
-  OS << '\n';
-
-  // ArrayRef<...> llvm::<ns>::GetLeafConstructs(llvm::<ns>::Directive Dir)
-  OS << "llvm::ArrayRef<" << DirectiveTypeName
-     << "> llvm::" << DirLang.getCppNamespace() << "::getLeafConstructs("
-     << DirectiveTypeName << " Dir) ";
-  OS << "{\n";
-
-  // Generate the locals.
-  for (Record *R : DirLang.getDirectives()) {
-    Directive Dir{R};
+  // Each row consists of
+  // - the id of the directive itself,
+  // - number of leaf constructs that will follow (0 for leafs),
+  // - ids of the leaf constructs (none if the directive is itself a leaf).
+  // The total number of these entries is at most MaxLeafCount+2. If this
+  // number is less than that, it is padded to occupy exactly MaxLeafCount+2
+  // entries in memory.
+  //
+  // The rows are stored in the table in the lexicographical order. This
+  // is intended to enable binary search when mapping a sequence of leafs
+  // back to the compound directive.
+  // The consequence of that is that in order to find a row corresponding
+  // to the given directive, we'd need to scan the first element of each
+  // row. To avoid this, an auxiliary ordering table is created, such that
+  //   row for Dir_A = table[auxiliary[Dir_A]].
+
+  std::vector<Record *> Directives = DirLang.getDirectives();
+  DenseMap<Record *, size_t> DirId; // Record * -> llvm::omp::Directive
+
+  for (auto [Idx, Rec] : llvm::enumerate(Directives))
+    DirId.insert(std::make_pair(Rec, Idx));
+
+  using LeafList = std::vector<int>;
+  int MaxLeafCount = GetMaxLeafCount(DirLang);
+
+  // The initial leaf table, rows order is same as directive order.
+  std::vector<LeafList> LeafTable(Directives.size());
+  for (auto [Idx, Rec] : llvm::enumerate(Directives)) {
+    Directive Dir{Rec};
+    std::vector<Record *> Leaves = Dir.getLeafConstructs();
+
+    auto &List = LeafTable[Idx];
+    List.resize(MaxLeafCount + 2);
+    List[0] = Idx;           // The id of the directive itself.
+    List[1] = Leaves.size(); // The number of leaves to follow.
+
+    for (int I = 0; I != MaxLeafCount; ++I)
+      List[I + 2] =
+          static_cast<size_t>(I) < Leaves.size() ? DirId.at(Leaves[I]) : -1;
+  }
 
-    std::vector<Record *> LeafConstructs = Dir.getLeafConstructs();
-    if (LeafConstructs.empty())
-      continue;
+  // Avoid sorting the vector<vector> array, instead sort an index array.
+  // It will also be useful later to create the auxiliary indexing array.
+  std::vector<int> Ordering(Directives.size());
+  std::iota(Ordering.begin(), Ordering.end(), 0);
+
+  llvm::sort(Ordering, [&](int A, int B) {
+    auto &LeavesA = LeafTable[A];
+    auto &LeavesB = LeafTable[B];
+    if (LeavesA[1] == 0 && LeavesB[1] == 0)
+      return LeavesA[0] < LeavesB[0];
+    return std::lexicographical_compare(&LeavesA[2], &LeavesA[2] + LeavesA[1],
+                                        &LeavesB[2], &LeavesB[2] + LeavesB[1]);
+  });
 
-    std::string ListName = "leafConstructs_" + Dir.getFormattedName();
-    OS << "  static const " << DirectiveTypeName << ' ' << ListName
-       << "[] = {\n";
-    for (Record *L : LeafConstructs) {
-      Directive LeafDir{L};
-      OS << "    " << getQualifiedName(LeafDir.getFormattedName()) << ",\n";
+  // Emit the table
+
+  // The directives are emitted into a scoped enum, for which the underlying
+  // type is `int` (by default). The code above uses `int` to store directive
+  // ids, so make sure that we catch it when something changes in the
+  // underlying type.
+  std::string DirectiveType = GetDirectiveType(DirLang);
+  OS << "static_assert(sizeof(" << DirectiveType << ") == sizeof(int));\n";
+
+  OS << "[[maybe_unused]] static const " << DirectiveType << ' ' << TableName
+     << "[][" << MaxLeafCount + 2 << "] = {\n";
+  for (size_t I = 0, E = Directives.size(); I != E; ++I) {
+    auto &Leaves = LeafTable[Ordering[I]];
+    OS << "    " << GetDirectiveName(DirLang, Directives[Leaves[0]]);
+    OS << ", static_cast<" << DirectiveType << ">(" << Leaves[1] << "),";
+    for (size_t I = 2, E = Leaves.size(); I != E; ++I) {
+      int Idx = Leaves[I];
+      if (Idx >= 0)
+        OS << ' ' << GetDirectiveName(DirLang, Directives[Leaves[I]]) << ',';
+      else
+        OS << " static_cast<" << DirectiveType << ">(-1),";
     }
-    OS << "  };\n";
-    ListNames.insert(std::make_pair(R, std::move(ListName)));
-  }
-
-  if (!ListNames.empty())
     OS << '\n';
-  OS << "  switch (Dir) {\n";
-  for (Record *R : DirLang.getDirectives()) {
-    auto F = ListNames.find(R);
-    if (F == ListNames.end())
-      continue;
-
-    Directive Dir{R};
-    OS << "  case " << getQualifiedName(Dir.getFormattedName()) << ":\n";
-    OS << "    return " << F->second << ";\n";
   }
-  OS << "  default:\n";
-  OS << "    return ArrayRef<" << DirectiveTypeName << ">{};\n";
-  OS << "  } // switch (Dir)\n";
-  OS << "}\n";
+  OS << "};\n\n";
+
+  // Emit the auxiliary index table: it's the inverse of the `Ordering`
+  // table above.
+  OS << "[[maybe_unused]] static const int " << TableName << "Ordering[] = {\n";
+  OS << "   ";
+  std::vector<int> Reverse(Ordering.size());
+  for (int I = 0, E = Ordering.size(); I != E; ++I)
+    Reverse[Ordering[I]] = I;
+  for (int Idx : Reverse)
+    OS << ' ' << Idx << ',';
+  OS << "\n};\n";
 }
 
 static void GenerateGetDirectiveAssociation(const DirectiveLanguage &DirLang,
@@ -1105,11 +1159,11 @@ void EmitDirectivesBasicImpl(const DirectiveLanguage &DirLang,
   // isAllowedClauseForDirective(Directive D, Clause C, unsigned Version)
   GenerateIsAllowedClause(DirLang, OS);
 
-  // getLeafConstructs(Directive D)
-  GenerateGetLeafConstructs(DirLang, OS);
-
   // getDirectiveAssociation(Directive D)
   GenerateGetDirectiveAssociation(DirLang, OS);
+
+  // Leaf table for getLeafConstructs, etc.
+  EmitLeafTable(DirLang, OS, "LeafConstructTable");
 }
 
 // Generate the implemenation section for the enumeration in the directive

Emit a special leaf constuct table in DirectiveEmitter.cpp, which will
allow both decomposition of a construct into leafs, and composition of
constituent constructs into a single compound construct (is possible).
@kparzysz kparzysz force-pushed the users/kparzysz/spr/a06-leafsorcomposite branch from 2fec998 to 291dc48 Compare April 1, 2024 16:30
Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks reasonable if we want to both split a construct into leaf constructs and also merge multiple constructs into a single compound one, if it exists. I'm just wondering if we need that much flexibility (and complexity associated with it).

It looks to me that there are mainly a couple of things that we want to be able to do:

  • Split any compound directive into a list of leaves, so that clauses can be appropriately assigned to each.
  • Split combined directives while preserving composite constructs as a unit, useful during lowering.

This approach works well for the first case and the second case can be made to work by first processing composite and standalone constructs, then splitting combined directives into the first leaf and the rest, which we would combine. Then processing that "rest of the construct" by processing it the same way and splitting it iteratively until everything has been processed. This has the disadvantage of being costly in comparison, since combining leaf constructs involves a binary search within the leaf table.

I'm wondering if there's a better abstraction we could use in tablegen that would work for both. Specifically, I'm thinking whether this makes sense to do:

  • Non-compound constructs remain as they are.
  • Rename the leafConstructs list of records to composite, and only use it for composite constructs.
  • Add an optional combined pair of records that defines exclusively for combined constructs what the outer/parent construct is and what the "rest" is.
  • A directive cannot define both composite and combined simultaneously.

An example of how that would look in tablegen could be:

def OMP_DoSimd : Directive<"do simd"> {
  let allowedClauses = [...];
  // 2+ elements: all leaf constructs.
  let composite = [OMP_Do, OMP_Simd];
}
def OMP_ParallelDoSimd : Directive<"parallel do simd"> {
  let allowedClauses = [...];
  // Always 2 elements: the first is a leaf and the second can be compound.
  let combined = [OMP_Parallel, OMP_DoSimd];
}
def OMP_TargetParallelDoSimd : Directive<"target parallel do simd"> {
  let allowedClauses = [...];
  // Always 2 elements: the first is a leaf and the second can be compound.
  let combined = [OMP_Target, OMP_ParallelDoSimd];
}

Based on that alternative representation, I think it should be easier to implement both types of splitting without having to allow re-combining leaf constructs back into compound ones. Another advantage is that the representation is explicit in the separation between combined and composite directives. However, it doesn't make it any less cumbersome to add the proposed list of new combined constructs.

This is just an idea, which may have some big disadvantages I haven't thought about. I won't block the current proposal, but I prefer if others more involved in the definition of the OMP.td format gave this a look before merging as well.

llvm/lib/Frontend/OpenMP/OMP.cpp Outdated Show resolved Hide resolved
llvm/lib/Frontend/OpenMP/OMP.cpp Show resolved Hide resolved
llvm/unittests/Frontend/OpenMPComposeTest.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@kparzysz kparzysz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have considered putting more information in the .td file, but I decided against it. The main reason was that TableGen would need to have a lot more knowledge about OMP directives than it does now:

  • It would need to be aware of OpenMP's definitions of "composite" vs. "combined" directives. This information would need to be encoded in the directive emitter (as a part of TableGen), plus it would also be in the OMP.cpp file (as a part of the compiler). In my view, the .td file should be easy to update (or in other words hard to make a mistake in).
  • The directive emitter is shared between OpenMP and OpenACC, and this change would mean that we need to distinguish these two targets in the emitters.

Even if there was 1000 directives to search through, the search is binary, requiring in the worst case ~10 steps.

Edit: What I considered was that TableGen would automatically emit composite sub-constructs. One of the ideas in your comment was to require the user to provide combined/composite constituents. In my opinion, this would be error-prone, since we'd have to take the user inputs at the face value, and it would be easy to make a mistake there. On the other hand, if were to automate the input generation, we'd need to express the rules in software, in the additional tool (which goes back to the argument of having the information encoded in multiple places).

llvm/lib/Frontend/OpenMP/OMP.cpp Show resolved Hide resolved
llvm/unittests/Frontend/OpenMPComposeTest.cpp Outdated Show resolved Hide resolved
llvm/lib/Frontend/OpenMP/OMP.cpp Outdated Show resolved Hide resolved
@skatrak
Copy link
Contributor

skatrak commented Apr 2, 2024

Thank you @kparzysz for sharing your thoughts. I think it's a reasonable solution, so it's fine by me. I'll leave it to someone more familiar with the directive emitter to approve the PR though.

@kparzysz kparzysz force-pushed the users/kparzysz/spr/a05-makeclause branch from f725fac to de93771 Compare April 17, 2024 14:30
Base automatically changed from users/kparzysz/spr/a05-makeclause to main April 18, 2024 17:02
@kparzysz kparzysz requested a review from tblah April 18, 2024 18:01
Copy link

github-actions bot commented Apr 18, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@skatrak skatrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this again and I just have a couple of minimal nits at this point. Flang/OpenMP changes LGTM, though I'm not familiar enough with the DirectiveEmitter to comment on it (maybe @alexey-bataev can comment on these changes or tag a suitable reviewer, since he reviewed similar changes before).

const ObjectList &objects,
const List<Clause> &clauses, mlir::Location loc,
llvm::SmallVectorImpl<mlir::Value> &operandRange) {
if (!objects.empty())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This check is probably redundant now, since genObjectList would loop over zero elements and do nothing.

@@ -1311,8 +1298,8 @@ static void genWsloopClauses(
if (ReductionProcessor::doReductionByRef(clauseOps.reductionVars))
clauseOps.reductionByRefAttr = firOpBuilder.getUnitAttr();

if (endClauses) {
ClauseProcessor ecp(converter, semaCtx, *endClauses);
if (!endClauses.empty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This check can be removed now.

ArrayRef<Directive> getLeafConstructs(Directive D) {
auto Idx = static_cast<std::size_t>(D);
if (Idx >= Directive_enumSize)
return {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return {};
return std::nullopt;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (Idx >= Directive_enumSize)
return {};
const auto *Row = LeafConstructTable[LeafConstructTableOrdering[Idx]];
return ArrayRef(&Row[2], &Row[2] + static_cast<int>(Row[1]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ArrayRef(&Row[2], &Row[2] + static_cast<int>(Row[1]));
return ArrayRef(&Row[2], Row[1]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, but I had to keep the static_cast.

auto Iter = std::lower_bound(
LeafConstructTable, LeafConstructTableEndDirective,
static_cast<std::decay_t<decltype(*LeafConstructTable)>>(RawLeafs.data()),
[](const auto *RowA, const auto *RowB) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand auto here, if it is short enough

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I kept the autos below because they trivially follow the types of RowA and RowB.

Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@kparzysz
Copy link
Contributor Author

The Windows build failure is due to running out of memory:

fatal error C1060: compiler is out of heap space

@kparzysz kparzysz merged commit 40137ff into main Apr 22, 2024
3 of 4 checks passed
@kparzysz kparzysz deleted the users/kparzysz/spr/a06-leafsorcomposite branch April 22, 2024 19:41
@ilovepi
Copy link
Contributor

ilovepi commented Apr 22, 2024

Hi, we're seeing test failures after this patch in our Linux CI (https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8749923140697717361/overview).

Both LLVM :: TableGen/directive1.td and LLVM :: TableGen/directive2.td are failing as of at least 14e6f63e. It may have always been broken, but a build failure seems to have hidden the initial problem.

Can you please take a look and revert if this will take a while to fix?

Error output:

Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /b/s/w/ir/x/w/llvm_build/bin/llvm-tblgen -gen-directive-decl -I /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/../../include /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td | /b/s/w/ir/x/w/llvm_build/bin/FileCheck -match-full-lines /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td
+ /b/s/w/ir/x/w/llvm_build/bin/FileCheck -match-full-lines /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td
+ /b/s/w/ir/x/w/llvm_build/bin/llvm-tblgen -gen-directive-decl -I /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/../../include /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td
RUN: at line 2: /b/s/w/ir/x/w/llvm_build/bin/llvm-tblgen -gen-directive-impl -I /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/../../include /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td | /b/s/w/ir/x/w/llvm_build/bin/FileCheck -match-full-lines /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td -check-prefix=IMPL
+ /b/s/w/ir/x/w/llvm_build/bin/llvm-tblgen -gen-directive-impl -I /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/../../include /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td
+ /b/s/w/ir/x/w/llvm_build/bin/FileCheck -match-full-lines /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td -check-prefix=IMPL
/b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td:304:15: error: IMPL-NEXT: expected string not found in input
// IMPL-NEXT: llvm::tdl::TDLD_dira, static_cast<llvm::tdl::Directive>(0),
              ^
<stdin>:243:79: note: scanning from here
[[maybe_unused]] static const llvm::tdl::Directive LeafConstructTable[][2] = {
                                                                              ^
<stdin>:244:2: note: possible intended match here
 {llvm::tdl::TDLD_dira, static_cast<llvm::tdl::Directive>(0),},
 ^

Input file: <stdin>
Check file: /b/s/w/ir/x/w/llvm-llvm-project/llvm/test/TableGen/directive2.td

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            .
            .
            .
          238:  } // switch(Dir) 
          239:  llvm_unreachable("Unexpected directive"); 
          240: } 
          241:  
          242: static_assert(sizeof(llvm::tdl::Directive) == sizeof(int)); 
          243: [[maybe_unused]] static const llvm::tdl::Directive LeafConstructTable[][2] = { 
next:304'0                                                                                   X error: no match found
          244:  {llvm::tdl::TDLD_dira, static_cast<llvm::tdl::Directive>(0),}, 
next:304'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
next:304'1      ?                                                               possible intended match
          245: }; 
next:304'0     ~~~
          246:  
next:304'0     ~
          247: [[maybe_unused]] static auto LeafConstructTableEndDirective = LeafConstructTable + 1; 
next:304'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          248:  
next:304'0     ~
          249: [[maybe_unused]] static const int LeafConstructTableOrdering[] = { 
next:304'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            .
            .
            .
>>>>>>

--

dyung added a commit that referenced this pull request Apr 22, 2024
…e tests to fail on multiple bots. (#89689)

Update the check lines added in #87247 after 14e6f63 updated the output
causing the tests to fail.

This should hopefully unbreak the bots failing due to these two tests
failing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:openmp OpenMP related changes to Clang flang:openmp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants