[Mlir-commits] [mlir] [MLIR][OpenMP] Add TableGen pseudo-generator for OpenMP-specific verification (PR #95552)
Sergio Afonso
llvmlistbot at llvm.org
Wed Jun 26 08:45:36 PDT 2024
https://github.com/skatrak updated https://github.com/llvm/llvm-project/pull/95552
>From b65d015b7116eff0a4057c4871e34a1226621861 Mon Sep 17 00:00:00 2001
From: Sergio Afonso <safonsof at amd.com>
Date: Fri, 14 Jun 2024 14:41:05 +0100
Subject: [PATCH 1/2] [MLIR][OpenMP] Add TableGen pseudo-generator for
OpenMP-specific verification
The introduction of the clause-based approach to defining OpenMP operations can
make it more difficult to detect and address certain programming errors derived
from this change. Specifically, it's possible for an operation to inadvertently
override otherwise automatically-populated properties and result in unexpected
and difficult to debug errors or incomplete operation definitions.
This patch introduces a TableGen backend that doesn't produce any output, but
rather only checks for these potential oversights in the definition of OpenMP
dialect operations and flags them as warnings or errors. This provides
descriptive and early feedback before any code is attempted to be generated for
these problematic definitions.
---
.../mlir/Dialect/OpenMP/CMakeLists.txt | 1 +
.../mlir/Dialect/OpenMP/OpenMPOpBase.td | 2 +
mlir/test/mlir-tblgen/openmp-ops-verify.td | 143 +++++++++++++++++
mlir/tools/mlir-tblgen/CMakeLists.txt | 1 +
mlir/tools/mlir-tblgen/OmpOpGen.cpp | 146 ++++++++++++++++++
5 files changed, 293 insertions(+)
create mode 100644 mlir/test/mlir-tblgen/openmp-ops-verify.td
create mode 100644 mlir/tools/mlir-tblgen/OmpOpGen.cpp
diff --git a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
index 419e24a733536..fbde2def9e0b2 100644
--- a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
@@ -3,6 +3,7 @@ mlir_tablegen(OmpCommon.td --gen-directive-decl --directives-dialect=OpenMP)
add_public_tablegen_target(omp_common_td)
set(LLVM_TARGET_DEFINITIONS OpenMPOps.td)
+mlir_tablegen(no-output -verify-openmp-ops)
mlir_tablegen(OpenMPOpsDialect.h.inc -gen-dialect-decls -dialect=omp)
mlir_tablegen(OpenMPOpsDialect.cpp.inc -gen-dialect-defs -dialect=omp)
mlir_tablegen(OpenMPOps.h.inc -gen-op-decls)
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpBase.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpBase.td
index d93abd63977ef..83e23599b37f6 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOpBase.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOpBase.td
@@ -135,6 +135,8 @@ class OpenMP_Op<string mnemonic, list<Trait> traits = [],
traits
)
)> {
+ list<OpenMP_Clause> clauseList = clauses;
+
// Aggregate `arguments` fields of all clauses into a single dag, to be used
// by operations to populate their `arguments` field.
defvar argsFilteredClauses =
diff --git a/mlir/test/mlir-tblgen/openmp-ops-verify.td b/mlir/test/mlir-tblgen/openmp-ops-verify.td
new file mode 100644
index 0000000000000..7fc962b4eaa4d
--- /dev/null
+++ b/mlir/test/mlir-tblgen/openmp-ops-verify.td
@@ -0,0 +1,143 @@
+// Tablegen tests for the verification of clause-based OpenMP dialect operation
+// definitions.
+
+// Run tablegen to generate OmpCommon.td in temp directory first.
+// RUN: mkdir -p %t/mlir/Dialect/OpenMP
+// RUN: mlir-tblgen --gen-directive-decl --directives-dialect=OpenMP \
+// RUN: %S/../../../llvm/include/llvm/Frontend/OpenMP/OMP.td \
+// RUN: -I %S/../../../llvm/include > %t/mlir/Dialect/OpenMP/OmpCommon.td
+
+// RUN: not mlir-tblgen -verify-openmp-ops -I %S/../../include -I %t %s 2>&1 | FileCheck %s
+
+include "mlir/Dialect/OpenMP/OpenMPOpBase.td"
+
+
+def OpenMP_SimpleClause : OpenMP_Clause<
+ /*isRequired=*/true, /*traits=*/false, /*arguments=*/false,
+ /*assemblyFormat=*/false, /*description=*/false,
+ /*extraClassDeclaration=*/false> {
+ let arguments = (ins I32:$val1);
+ let assemblyFormat = "`val1` `(` $val1 `)`";
+ let description = "Simple clause description.";
+ let extraClassDeclaration = "void simpleClauseExtraClassDecl();";
+}
+
+
+// -----------------------------------------------------------------------------
+// Verify errors / warnings for overriding each field.
+// -----------------------------------------------------------------------------
+
+def 1OverrideArgsOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
+ let description = "Description of operation." # clausesDescription;
+ dag arguments = (ins I32:$myval);
+}
+// CHECK: warning: 'Simple' clause-defined argument 'I32:$val1' not present in operation.
+// CHECK-SAME: Consider `dag arguments = !con(clausesArgs, ...)` or explicitly skipping this field.
+// CHECK-NEXT: def 1OverrideArgsOp
+
+def 2OverrideAssemblyFormatOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
+ let description = "Description of operation." # clausesDescription;
+ string assemblyFormat = "`alt_repr` `(` $val1 `)`";
+}
+// CHECK: warning: 'Simple' clause-defined `assemblyFormat` not present in operation.
+// CHECK-SAME: Consider concatenating `clausesAssemblyFormat` or explicitly skipping this field.
+// CHECK-NEXT: def 2OverrideAssemblyFormatOp
+
+def 3OverrideDescriptionOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
+ let description = "Description of operation.";
+}
+// CHECK: error: 'Simple' clause-defined `description` not present in operation.
+// CHECK-SAME: Consider concatenating `clausesDescription` or explicitly skipping this field.
+// CHECK-NEXT: def 3OverrideDescriptionOp
+
+def 4OverrideExtraClassDeclarationOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
+ let description = "Description of operation." # clausesDescription;
+ string extraClassDeclaration = "";
+}
+// CHECK: warning: 'Simple' clause-defined `extraClassDeclaration` not present in operation.
+// CHECK-SAME: Consider concatenating `clausesExtraClassDeclaration` or explicitly skipping this field.
+// CHECK-NEXT: def 4OverrideExtraClassDeclarationOp
+
+
+// -----------------------------------------------------------------------------
+// Verify that reporting is correct when OpenMP_Clause is inherited indirectly.
+// -----------------------------------------------------------------------------
+
+class OpenMP_IndirectClauseSkip<
+ bit traits = false, bit arguments = false, bit assemblyFormat = false,
+ bit description = false, bit extraClassDeclaration = false
+ > : OpenMP_Clause</*isRequired=*/true, traits, arguments, assemblyFormat,
+ description, extraClassDeclaration> {
+ let arguments = (ins I32:$val2);
+ let assemblyFormat = "`val2` `(` $val2 `)`";
+ let description = "Indirectly-inherited clause description.";
+ let extraClassDeclaration = "void indirectClauseExtraClassDecl();";
+}
+
+def IndirectClause : OpenMP_IndirectClauseSkip<>;
+
+def 5IndirectClauseOp : OpenMP_Op<"op", clauses=[IndirectClause]> {
+ let description = "Description of operation." # clausesDescription;
+ dag arguments = (ins I32:$myval);
+}
+// CHECK: warning: 'Indirect' clause-defined argument 'I32:$val2' not present in operation.
+// CHECK-NEXT: def 5IndirectClauseOp
+
+
+// -----------------------------------------------------------------------------
+// Verify that multiple clauses are taken into account.
+// -----------------------------------------------------------------------------
+
+def 6MultiClauseOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause, IndirectClause]> {
+ let description = "Description of operation." # clausesDescription;
+ let arguments = (ins I32:$val1);
+ let assemblyFormat = "`val2` `(` $val2 `)`";
+}
+// CHECK: warning: 'Simple' clause-defined `assemblyFormat` not present in operation.
+// CHECK-NEXT: def 6MultiClauseOp
+// CHECK: warning: 'Indirect' clause-defined argument 'I32:$val2' not present in operation.
+// CHECK-NEXT: def 6MultiClauseOp
+
+
+// -----------------------------------------------------------------------------
+// Verify that reporting is correct when clause definitions have other
+// superclasses in addition to OpenMP_Clause.
+// -----------------------------------------------------------------------------
+
+class Placeholder {}
+def MultiSuperClassClause : Placeholder, OpenMP_IndirectClauseSkip<>;
+
+def 7MultiSuperClassClauseOp : OpenMP_Op<"op", clauses=[IndirectClause]> {
+ let description = "Description of operation." # clausesDescription;
+ dag arguments = (ins I32:$myval);
+}
+// CHECK: warning: 'Indirect' clause-defined argument 'I32:$val2' not present in operation.
+// CHECK-NEXT: def 7MultiSuperClassClauseOp
+
+
+// -----------------------------------------------------------------------------
+// Verify that no errors are produced if the field being overriden is also
+// skipped for the clause.
+// -----------------------------------------------------------------------------
+
+def SkipArgsOp : OpenMP_Op<"op",
+ clauses=[OpenMP_IndirectClauseSkip<arguments=true>]> {
+ let description = "Description of operation." # clausesDescription;
+ dag arguments = (ins I32:$myval);
+}
+def SkipAssemblyFormatOp : OpenMP_Op<"op",
+ clauses=[OpenMP_IndirectClauseSkip<assemblyFormat=true>]> {
+ let description = "Description of operation." # clausesDescription;
+ string assemblyFormat = "`alt_repr` `(` $val1 `)`";
+}
+def SkipDescriptionOp : OpenMP_Op<"op",
+ clauses=[OpenMP_IndirectClauseSkip<description=true>]> {
+ let description = "Description of operation.";
+}
+def SkipExtraClassDeclarationOp : OpenMP_Op<"op",
+ clauses=[OpenMP_IndirectClauseSkip<extraClassDeclaration=true>]> {
+ let description = "Description of operation." # clausesDescription;
+ string extraClassDeclaration = "";
+}
+// CHECK-NOT: error:
+// CHECK-NOT: warning:
diff --git a/mlir/tools/mlir-tblgen/CMakeLists.txt b/mlir/tools/mlir-tblgen/CMakeLists.txt
index 20a200bc35408..fb507dc7f8c3c 100644
--- a/mlir/tools/mlir-tblgen/CMakeLists.txt
+++ b/mlir/tools/mlir-tblgen/CMakeLists.txt
@@ -19,6 +19,7 @@ add_tablegen(mlir-tblgen MLIR
LLVMIRConversionGen.cpp
LLVMIRIntrinsicGen.cpp
mlir-tblgen.cpp
+ OmpOpGen.cpp
OpClass.cpp
OpDefinitionsGen.cpp
OpDocGen.cpp
diff --git a/mlir/tools/mlir-tblgen/OmpOpGen.cpp b/mlir/tools/mlir-tblgen/OmpOpGen.cpp
new file mode 100644
index 0000000000000..a9ba86a777d8e
--- /dev/null
+++ b/mlir/tools/mlir-tblgen/OmpOpGen.cpp
@@ -0,0 +1,146 @@
+//===- OmpOpGen.cpp - OpenMP dialect op specific generators ---------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+// OmpOpGen defines OpenMP dialect operation specific generators.
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/TableGen/GenInfo.h"
+
+#include "llvm/TableGen/Error.h"
+#include "llvm/TableGen/Record.h"
+
+using namespace llvm;
+
+static StringRef extractOmpClauseName(Record *clause) {
+ Record *ompClause = clause->getRecords().getClass("OpenMP_Clause");
+ assert(ompClause && "base OpenMP records expected to be defined");
+
+ StringRef clauseClassName;
+ SmallVector<Record *, 1> clauseSuperClasses;
+ clause->getDirectSuperClasses(clauseSuperClasses);
+
+ // Check if OpenMP_Clause is a direct superclass.
+ for (Record *superClass : clauseSuperClasses) {
+ if (superClass == ompClause) {
+ clauseClassName = clause->getName();
+ break;
+ }
+ }
+
+ // Support indirectly-inherited OpenMP_Clauses.
+ if (clauseClassName.empty()) {
+ for (auto [superClass, _] : clause->getSuperClasses()) {
+ if (superClass->isSubClassOf(ompClause)) {
+ clauseClassName = superClass->getName();
+ break;
+ }
+ }
+ }
+
+ assert(!clauseClassName.empty() && "clause name must be found");
+
+ // Keep only the OpenMP clause name itself for reporting purposes.
+ StringRef prefix = "OpenMP_";
+ StringRef suffixes[] = {"Skip", "Clause"};
+
+ if (clauseClassName.starts_with(prefix))
+ clauseClassName = clauseClassName.substr(prefix.size());
+
+ for (StringRef suffix : suffixes) {
+ if (clauseClassName.ends_with(suffix))
+ clauseClassName =
+ clauseClassName.substr(0, clauseClassName.size() - suffix.size());
+ }
+
+ return clauseClassName;
+}
+
+static bool verifyArgument(DagInit *arguments, StringRef argName,
+ Init *argInit) {
+ auto range = zip_equal(arguments->getArgNames(), arguments->getArgs());
+ return std::find_if(
+ range.begin(), range.end(),
+ [&](std::tuple<llvm::StringInit *const &, llvm::Init *const &> v) {
+ return std::get<0>(v)->getAsUnquotedString() == argName &&
+ std::get<1>(v) == argInit;
+ }) != range.end();
+}
+
+static bool verifyStringValue(StringRef value, Record *op, Record *clause) {
+ auto opValue = op->getValueAsOptionalString(value);
+ auto clauseValue = clause->getValueAsOptionalString(value);
+ if (!opValue)
+ return !clauseValue || clauseValue->empty();
+
+ return !clauseValue || opValue->contains(clauseValue->trim());
+}
+
+// Verify that all fields of the given clause not explicitly ignored are present
+// in the corresponding operation field.
+static void verifyClause(Record *op, Record *clause) {
+ StringRef clauseClassName = extractOmpClauseName(clause);
+
+ if (!clause->getValueAsBit("ignoreArgs")) {
+ DagInit *opArguments = op->getValueAsDag("arguments");
+ DagInit *arguments = clause->getValueAsDag("arguments");
+
+ for (auto [name, arg] :
+ zip(arguments->getArgNames(), arguments->getArgs())) {
+ if (!verifyArgument(opArguments, name->getAsUnquotedString(), arg))
+ PrintWarning(
+ op->getLoc(),
+ "'" + clauseClassName + "' clause-defined argument '" +
+ arg->getAsUnquotedString() + ":$" +
+ name->getAsUnquotedString() +
+ "' not present in operation. Consider `dag arguments = "
+ "!con(clausesArgs, ...)` or explicitly skipping this field.");
+ }
+ }
+
+ if (!clause->getValueAsBit("ignoreAsmFormat") &&
+ !verifyStringValue("assemblyFormat", op, clause))
+ PrintWarning(
+ op->getLoc(),
+ "'" + clauseClassName +
+ "' clause-defined `assemblyFormat` not present in operation. "
+ "Consider concatenating `clausesAssemblyFormat` or explicitly "
+ "skipping this field.");
+
+ if (!clause->getValueAsBit("ignoreDesc") &&
+ !verifyStringValue("description", op, clause))
+ PrintError(op->getLoc(),
+ "'" + clauseClassName +
+ "' clause-defined `description` not present in operation. "
+ "Consider concatenating `clausesDescription` or explicitly "
+ "skipping this field.");
+
+ if (!clause->getValueAsBit("ignoreExtraDecl") &&
+ !verifyStringValue("extraClassDeclaration", op, clause))
+ PrintWarning(
+ op->getLoc(),
+ "'" + clauseClassName +
+ "' clause-defined `extraClassDeclaration` not present in "
+ "operation. Consider concatenating `clausesExtraClassDeclaration` "
+ "or explicitly skipping this field.");
+}
+
+static bool verifyDecls(const RecordKeeper &recordKeeper, raw_ostream &) {
+ for (Record *op : recordKeeper.getAllDerivedDefinitions("OpenMP_Op")) {
+ for (Record *clause : op->getValueAsListOfDefs("clauseList"))
+ verifyClause(op, clause);
+ }
+
+ return false;
+}
+
+// Registers the generator to mlir-tblgen.
+static mlir::GenRegistration
+ verifyOpenmpOps("verify-openmp-ops",
+ "Verify OpenMP operations (produce no output file)",
+ verifyDecls);
>From 89a510809780bfc8f6b656dd35d21882e77da312 Mon Sep 17 00:00:00 2001
From: Sergio Afonso <safonsof at amd.com>
Date: Wed, 26 Jun 2024 16:37:23 +0100
Subject: [PATCH 2/2] Address review comments
---
.../mlir/Dialect/OpenMP/CMakeLists.txt | 9 ++++++
mlir/tools/mlir-tblgen/OmpOpGen.cpp | 30 +++++++++++++++----
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
index fbde2def9e0b2..d3422f6e48b06 100644
--- a/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt
@@ -3,7 +3,16 @@ mlir_tablegen(OmpCommon.td --gen-directive-decl --directives-dialect=OpenMP)
add_public_tablegen_target(omp_common_td)
set(LLVM_TARGET_DEFINITIONS OpenMPOps.td)
+
+# Run the OpenMP verifier tablegen pseudo-backend while preventing the produced
+# dummy output from being added as a dependency to any tablegen targets defined
+# below.
+set(TABLEGEN_OUTPUT_TMP ${TABLEGEN_OUTPUT})
mlir_tablegen(no-output -verify-openmp-ops)
+file(REMOVE ${CMAKE_CURRENT_BINARY_DIR}/no-output ${CMAKE_CURRENT_BINARY_DIR}/no-output.d)
+set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT_TMP})
+unset(TABLEGEN_OUTPUT_TMP)
+
mlir_tablegen(OpenMPOpsDialect.h.inc -gen-dialect-decls -dialect=omp)
mlir_tablegen(OpenMPOpsDialect.cpp.inc -gen-dialect-defs -dialect=omp)
mlir_tablegen(OpenMPOps.h.inc -gen-op-decls)
diff --git a/mlir/tools/mlir-tblgen/OmpOpGen.cpp b/mlir/tools/mlir-tblgen/OmpOpGen.cpp
index a9ba86a777d8e..51eb43f322e6a 100644
--- a/mlir/tools/mlir-tblgen/OmpOpGen.cpp
+++ b/mlir/tools/mlir-tblgen/OmpOpGen.cpp
@@ -17,6 +17,13 @@
using namespace llvm;
+/// Obtain the name of the OpenMP clause a given record inheriting
+/// `OpenMP_Clause` refers to.
+///
+/// It supports direct and indirect `OpenMP_Clause` superclasses. Once the
+/// `OpenMP_Clause` class the record is based on is found, the optional
+/// "OpenMP_" prefix and "Skip" and "Clause" suffixes are removed to return only
+/// the clause name, i.e. "OpenMP_CollapseClauseSkip" is returned as "Collapse".
static StringRef extractOmpClauseName(Record *clause) {
Record *ompClause = clause->getRecords().getClass("OpenMP_Clause");
assert(ompClause && "base OpenMP records expected to be defined");
@@ -61,6 +68,8 @@ static StringRef extractOmpClauseName(Record *clause) {
return clauseClassName;
}
+/// Check that the given argument, identified by its name and initialization
+/// value, is present in the \c arguments `dag`.
static bool verifyArgument(DagInit *arguments, StringRef argName,
Init *argInit) {
auto range = zip_equal(arguments->getArgNames(), arguments->getArgs());
@@ -72,17 +81,26 @@ static bool verifyArgument(DagInit *arguments, StringRef argName,
}) != range.end();
}
+/// Check that the given string record value, identified by its name \c value,
+/// is either undefined or empty in both the given operation and clause record
+/// or its contents for the clause record are contained in the operation record.
static bool verifyStringValue(StringRef value, Record *op, Record *clause) {
auto opValue = op->getValueAsOptionalString(value);
auto clauseValue = clause->getValueAsOptionalString(value);
- if (!opValue)
- return !clauseValue || clauseValue->empty();
- return !clauseValue || opValue->contains(clauseValue->trim());
+ bool opHasValue = opValue && !opValue->trim().empty();
+ bool clauseHasValue = clauseValue && !clauseValue->trim().empty();
+
+ if (!opHasValue)
+ return !clauseHasValue;
+
+ return !clauseHasValue || opValue->contains(clauseValue->trim());
}
-// Verify that all fields of the given clause not explicitly ignored are present
-// in the corresponding operation field.
+/// Verify that all fields of the given clause not explicitly ignored are
+/// present in the corresponding operation field.
+///
+/// Print warnings or errors where this is not the case.
static void verifyClause(Record *op, Record *clause) {
StringRef clauseClassName = extractOmpClauseName(clause);
@@ -130,6 +148,8 @@ static void verifyClause(Record *op, Record *clause) {
"or explicitly skipping this field.");
}
+/// Verify that all properties of `OpenMP_Clause`s of records deriving from
+/// `OpenMP_Op`s have been inherited by the latter.
static bool verifyDecls(const RecordKeeper &recordKeeper, raw_ostream &) {
for (Record *op : recordKeeper.getAllDerivedDefinitions("OpenMP_Op")) {
for (Record *clause : op->getValueAsListOfDefs("clauseList"))
More information about the Mlir-commits
mailing list