Skip to content

Commit d5e4e70

Browse files
authored
[MLIR][OpenMP] Add TableGen pseudo-generator for OpenMP-specific verification (#95552)
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.
1 parent 0a95f2f commit d5e4e70

File tree

8 files changed

+328
-4
lines changed

8 files changed

+328
-4
lines changed

mlir/include/mlir/Dialect/OpenMP/CMakeLists.txt

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,16 @@ mlir_tablegen(OmpCommon.td --gen-directive-decl --directives-dialect=OpenMP)
33
add_public_tablegen_target(omp_common_td)
44

55
set(LLVM_TARGET_DEFINITIONS OpenMPOps.td)
6+
7+
# Run the OpenMP verifier tablegen pseudo-backend while preventing the produced
8+
# dummy output from being added as a dependency to any tablegen targets defined
9+
# below.
10+
set(TABLEGEN_OUTPUT_TMP ${TABLEGEN_OUTPUT})
11+
mlir_tablegen(no-output -verify-openmp-ops)
12+
file(REMOVE ${CMAKE_CURRENT_BINARY_DIR}/no-output ${CMAKE_CURRENT_BINARY_DIR}/no-output.d)
13+
set(TABLEGEN_OUTPUT ${TABLEGEN_OUTPUT_TMP})
14+
unset(TABLEGEN_OUTPUT_TMP)
15+
616
mlir_tablegen(OpenMPOpsDialect.h.inc -gen-dialect-decls -dialect=omp)
717
mlir_tablegen(OpenMPOpsDialect.cpp.inc -gen-dialect-defs -dialect=omp)
818
mlir_tablegen(OpenMPOps.h.inc -gen-op-decls)

mlir/include/mlir/Dialect/OpenMP/OpenMPOpBase.td

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,8 @@ class OpenMP_Op<string mnemonic, list<Trait> traits = [],
135135
traits
136136
)
137137
)> {
138+
list<OpenMP_Clause> clauseList = clauses;
139+
138140
// Aggregate `arguments` fields of all clauses into a single dag, to be used
139141
// by operations to populate their `arguments` field.
140142
defvar argsFilteredClauses =

mlir/test/lit.cfg.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@
4949

5050
config.substitutions.append(("%PATH%", config.environment["PATH"]))
5151
config.substitutions.append(("%shlibext", config.llvm_shlib_ext))
52+
config.substitutions.append(("%llvm_src_root", config.llvm_src_root))
5253
config.substitutions.append(("%mlir_src_root", config.mlir_src_root))
5354
config.substitutions.append(("%host_cxx", config.host_cxx))
5455
config.substitutions.append(("%host_cc", config.host_cc))

mlir/test/lit.site.cfg.py.in

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import sys
44

55
config.target_triple = "@LLVM_TARGET_TRIPLE@"
6+
config.llvm_src_root = "@LLVM_SOURCE_DIR@"
67
config.llvm_tools_dir = lit_config.substitute("@LLVM_TOOLS_DIR@")
78
config.llvm_shlib_ext = "@SHLIBEXT@"
89
config.llvm_shlib_dir = lit_config.substitute(path(r"@SHLIBDIR@"))
Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
// Tablegen tests for the verification of clause-based OpenMP dialect operation
2+
// definitions.
3+
4+
// Run tablegen to generate OmpCommon.td in temp directory first.
5+
// RUN: mkdir -p %t/mlir/Dialect/OpenMP
6+
// RUN: mlir-tblgen --gen-directive-decl --directives-dialect=OpenMP \
7+
// RUN: %llvm_src_root/include/llvm/Frontend/OpenMP/OMP.td \
8+
// RUN: -I %llvm_src_root/include > %t/mlir/Dialect/OpenMP/OmpCommon.td
9+
10+
// RUN: not mlir-tblgen -verify-openmp-ops -I %mlir_src_root/include -I %t %s 2>&1 | FileCheck %s
11+
12+
include "mlir/Dialect/OpenMP/OpenMPOpBase.td"
13+
14+
15+
def OpenMP_SimpleClause : OpenMP_Clause<
16+
/*isRequired=*/true, /*traits=*/false, /*arguments=*/false,
17+
/*assemblyFormat=*/false, /*description=*/false,
18+
/*extraClassDeclaration=*/false> {
19+
let arguments = (ins I32:$val1);
20+
let assemblyFormat = "`val1` `(` $val1 `)`";
21+
let description = "Simple clause description.";
22+
let extraClassDeclaration = "void simpleClauseExtraClassDecl();";
23+
}
24+
25+
26+
// -----------------------------------------------------------------------------
27+
// Verify errors / warnings for overriding each field.
28+
// -----------------------------------------------------------------------------
29+
30+
def 1OverrideArgsOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
31+
let description = "Description of operation." # clausesDescription;
32+
dag arguments = (ins I32:$myval);
33+
}
34+
// CHECK: warning: 'Simple' clause-defined argument 'I32:$val1' not present in operation.
35+
// CHECK-SAME: Consider `dag arguments = !con(clausesArgs, ...)` or explicitly skipping this field.
36+
// CHECK-NEXT: def 1OverrideArgsOp
37+
38+
def 2OverrideAssemblyFormatOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
39+
let description = "Description of operation." # clausesDescription;
40+
string assemblyFormat = "`alt_repr` `(` $val1 `)`";
41+
}
42+
// CHECK: warning: 'Simple' clause-defined `assemblyFormat` not present in operation.
43+
// CHECK-SAME: Consider concatenating `clausesAssemblyFormat` or explicitly skipping this field.
44+
// CHECK-NEXT: def 2OverrideAssemblyFormatOp
45+
46+
def 3OverrideDescriptionOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
47+
let description = "Description of operation.";
48+
}
49+
// CHECK: error: 'Simple' clause-defined `description` not present in operation.
50+
// CHECK-SAME: Consider concatenating `clausesDescription` or explicitly skipping this field.
51+
// CHECK-NEXT: def 3OverrideDescriptionOp
52+
53+
def 4OverrideExtraClassDeclarationOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause]> {
54+
let description = "Description of operation." # clausesDescription;
55+
string extraClassDeclaration = "";
56+
}
57+
// CHECK: warning: 'Simple' clause-defined `extraClassDeclaration` not present in operation.
58+
// CHECK-SAME: Consider concatenating `clausesExtraClassDeclaration` or explicitly skipping this field.
59+
// CHECK-NEXT: def 4OverrideExtraClassDeclarationOp
60+
61+
62+
// -----------------------------------------------------------------------------
63+
// Verify that reporting is correct when OpenMP_Clause is inherited indirectly.
64+
// -----------------------------------------------------------------------------
65+
66+
class OpenMP_IndirectClauseSkip<
67+
bit traits = false, bit arguments = false, bit assemblyFormat = false,
68+
bit description = false, bit extraClassDeclaration = false
69+
> : OpenMP_Clause</*isRequired=*/true, traits, arguments, assemblyFormat,
70+
description, extraClassDeclaration> {
71+
let arguments = (ins I32:$val2);
72+
let assemblyFormat = "`val2` `(` $val2 `)`";
73+
let description = "Indirectly-inherited clause description.";
74+
let extraClassDeclaration = "void indirectClauseExtraClassDecl();";
75+
}
76+
77+
def IndirectClause : OpenMP_IndirectClauseSkip<>;
78+
79+
def 5IndirectClauseOp : OpenMP_Op<"op", clauses=[IndirectClause]> {
80+
let description = "Description of operation." # clausesDescription;
81+
dag arguments = (ins I32:$myval);
82+
}
83+
// CHECK: warning: 'Indirect' clause-defined argument 'I32:$val2' not present in operation.
84+
// CHECK-NEXT: def 5IndirectClauseOp
85+
86+
87+
// -----------------------------------------------------------------------------
88+
// Verify that multiple clauses are taken into account.
89+
// -----------------------------------------------------------------------------
90+
91+
def 6MultiClauseOp : OpenMP_Op<"op", clauses=[OpenMP_SimpleClause, IndirectClause]> {
92+
let description = "Description of operation." # clausesDescription;
93+
let arguments = (ins I32:$val1);
94+
let assemblyFormat = "`val2` `(` $val2 `)`";
95+
}
96+
// CHECK: warning: 'Simple' clause-defined `assemblyFormat` not present in operation.
97+
// CHECK-NEXT: def 6MultiClauseOp
98+
// CHECK: warning: 'Indirect' clause-defined argument 'I32:$val2' not present in operation.
99+
// CHECK-NEXT: def 6MultiClauseOp
100+
101+
102+
// -----------------------------------------------------------------------------
103+
// Verify that reporting is correct when clause definitions have other
104+
// superclasses in addition to OpenMP_Clause.
105+
// -----------------------------------------------------------------------------
106+
107+
class Placeholder {}
108+
def MultiSuperClassClause : Placeholder, OpenMP_IndirectClauseSkip<>;
109+
110+
def 7MultiSuperClassClauseOp : OpenMP_Op<"op", clauses=[IndirectClause]> {
111+
let description = "Description of operation." # clausesDescription;
112+
dag arguments = (ins I32:$myval);
113+
}
114+
// CHECK: warning: 'Indirect' clause-defined argument 'I32:$val2' not present in operation.
115+
// CHECK-NEXT: def 7MultiSuperClassClauseOp
116+
117+
118+
// -----------------------------------------------------------------------------
119+
// Verify that no errors are produced if the field being overriden is also
120+
// skipped for the clause.
121+
// -----------------------------------------------------------------------------
122+
123+
def SkipArgsOp : OpenMP_Op<"op",
124+
clauses=[OpenMP_IndirectClauseSkip<arguments=true>]> {
125+
let description = "Description of operation." # clausesDescription;
126+
dag arguments = (ins I32:$myval);
127+
}
128+
def SkipAssemblyFormatOp : OpenMP_Op<"op",
129+
clauses=[OpenMP_IndirectClauseSkip<assemblyFormat=true>]> {
130+
let description = "Description of operation." # clausesDescription;
131+
string assemblyFormat = "`alt_repr` `(` $val1 `)`";
132+
}
133+
def SkipDescriptionOp : OpenMP_Op<"op",
134+
clauses=[OpenMP_IndirectClauseSkip<description=true>]> {
135+
let description = "Description of operation.";
136+
}
137+
def SkipExtraClassDeclarationOp : OpenMP_Op<"op",
138+
clauses=[OpenMP_IndirectClauseSkip<extraClassDeclaration=true>]> {
139+
let description = "Description of operation." # clausesDescription;
140+
string extraClassDeclaration = "";
141+
}
142+
// CHECK-NOT: error:
143+
// CHECK-NOT: warning:

mlir/test/mlir-tblgen/openmp-ops.td

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,11 @@
66
// Run tablegen to generate OmpCommon.td in temp directory first.
77
// RUN: mkdir -p %t/mlir/Dialect/OpenMP
88
// RUN: mlir-tblgen --gen-directive-decl --directives-dialect=OpenMP \
9-
// RUN: %S/../../../llvm/include/llvm/Frontend/OpenMP/OMP.td \
10-
// RUN: -I %S/../../../llvm/include > %t/mlir/Dialect/OpenMP/OmpCommon.td
9+
// RUN: %llvm_src_root/include/llvm/Frontend/OpenMP/OMP.td \
10+
// RUN: -I %llvm_src_root/include > %t/mlir/Dialect/OpenMP/OmpCommon.td
1111

12-
// RUN: mlir-tblgen -gen-op-decls -I %S/../../include -I %t %s | FileCheck %s --check-prefix=DECL
13-
// RUN: mlir-tblgen -gen-op-doc -I %S/../../include -I %t %s | FileCheck %s --check-prefix=DOC
12+
// RUN: mlir-tblgen -gen-op-decls -I %mlir_src_root/include -I %t %s | FileCheck %s --check-prefix=DECL
13+
// RUN: mlir-tblgen -gen-op-doc -I %mlir_src_root/include -I %t %s | FileCheck %s --check-prefix=DOC
1414

1515
include "mlir/Dialect/OpenMP/OpenMPOpBase.td"
1616

mlir/tools/mlir-tblgen/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ add_tablegen(mlir-tblgen MLIR
1919
LLVMIRConversionGen.cpp
2020
LLVMIRIntrinsicGen.cpp
2121
mlir-tblgen.cpp
22+
OmpOpGen.cpp
2223
OpClass.cpp
2324
OpDefinitionsGen.cpp
2425
OpDocGen.cpp

mlir/tools/mlir-tblgen/OmpOpGen.cpp

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
//===- OmpOpGen.cpp - OpenMP dialect op specific generators ---------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
//
9+
// OmpOpGen defines OpenMP dialect operation specific generators.
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
#include "mlir/TableGen/GenInfo.h"
14+
15+
#include "llvm/TableGen/Error.h"
16+
#include "llvm/TableGen/Record.h"
17+
18+
using namespace llvm;
19+
20+
/// Obtain the name of the OpenMP clause a given record inheriting
21+
/// `OpenMP_Clause` refers to.
22+
///
23+
/// It supports direct and indirect `OpenMP_Clause` superclasses. Once the
24+
/// `OpenMP_Clause` class the record is based on is found, the optional
25+
/// "OpenMP_" prefix and "Skip" and "Clause" suffixes are removed to return only
26+
/// the clause name, i.e. "OpenMP_CollapseClauseSkip" is returned as "Collapse".
27+
static StringRef extractOmpClauseName(Record *clause) {
28+
Record *ompClause = clause->getRecords().getClass("OpenMP_Clause");
29+
assert(ompClause && "base OpenMP records expected to be defined");
30+
31+
StringRef clauseClassName;
32+
SmallVector<Record *, 1> clauseSuperClasses;
33+
clause->getDirectSuperClasses(clauseSuperClasses);
34+
35+
// Check if OpenMP_Clause is a direct superclass.
36+
for (Record *superClass : clauseSuperClasses) {
37+
if (superClass == ompClause) {
38+
clauseClassName = clause->getName();
39+
break;
40+
}
41+
}
42+
43+
// Support indirectly-inherited OpenMP_Clauses.
44+
if (clauseClassName.empty()) {
45+
for (auto [superClass, _] : clause->getSuperClasses()) {
46+
if (superClass->isSubClassOf(ompClause)) {
47+
clauseClassName = superClass->getName();
48+
break;
49+
}
50+
}
51+
}
52+
53+
assert(!clauseClassName.empty() && "clause name must be found");
54+
55+
// Keep only the OpenMP clause name itself for reporting purposes.
56+
StringRef prefix = "OpenMP_";
57+
StringRef suffixes[] = {"Skip", "Clause"};
58+
59+
if (clauseClassName.starts_with(prefix))
60+
clauseClassName = clauseClassName.substr(prefix.size());
61+
62+
for (StringRef suffix : suffixes) {
63+
if (clauseClassName.ends_with(suffix))
64+
clauseClassName =
65+
clauseClassName.substr(0, clauseClassName.size() - suffix.size());
66+
}
67+
68+
return clauseClassName;
69+
}
70+
71+
/// Check that the given argument, identified by its name and initialization
72+
/// value, is present in the \c arguments `dag`.
73+
static bool verifyArgument(DagInit *arguments, StringRef argName,
74+
Init *argInit) {
75+
auto range = zip_equal(arguments->getArgNames(), arguments->getArgs());
76+
return std::find_if(
77+
range.begin(), range.end(),
78+
[&](std::tuple<llvm::StringInit *const &, llvm::Init *const &> v) {
79+
return std::get<0>(v)->getAsUnquotedString() == argName &&
80+
std::get<1>(v) == argInit;
81+
}) != range.end();
82+
}
83+
84+
/// Check that the given string record value, identified by its name \c value,
85+
/// is either undefined or empty in both the given operation and clause record
86+
/// or its contents for the clause record are contained in the operation record.
87+
static bool verifyStringValue(StringRef value, Record *op, Record *clause) {
88+
auto opValue = op->getValueAsOptionalString(value);
89+
auto clauseValue = clause->getValueAsOptionalString(value);
90+
91+
bool opHasValue = opValue && !opValue->trim().empty();
92+
bool clauseHasValue = clauseValue && !clauseValue->trim().empty();
93+
94+
if (!opHasValue)
95+
return !clauseHasValue;
96+
97+
return !clauseHasValue || opValue->contains(clauseValue->trim());
98+
}
99+
100+
/// Verify that all fields of the given clause not explicitly ignored are
101+
/// present in the corresponding operation field.
102+
///
103+
/// Print warnings or errors where this is not the case.
104+
static void verifyClause(Record *op, Record *clause) {
105+
StringRef clauseClassName = extractOmpClauseName(clause);
106+
107+
if (!clause->getValueAsBit("ignoreArgs")) {
108+
DagInit *opArguments = op->getValueAsDag("arguments");
109+
DagInit *arguments = clause->getValueAsDag("arguments");
110+
111+
for (auto [name, arg] :
112+
zip(arguments->getArgNames(), arguments->getArgs())) {
113+
if (!verifyArgument(opArguments, name->getAsUnquotedString(), arg))
114+
PrintWarning(
115+
op->getLoc(),
116+
"'" + clauseClassName + "' clause-defined argument '" +
117+
arg->getAsUnquotedString() + ":$" +
118+
name->getAsUnquotedString() +
119+
"' not present in operation. Consider `dag arguments = "
120+
"!con(clausesArgs, ...)` or explicitly skipping this field.");
121+
}
122+
}
123+
124+
if (!clause->getValueAsBit("ignoreAsmFormat") &&
125+
!verifyStringValue("assemblyFormat", op, clause))
126+
PrintWarning(
127+
op->getLoc(),
128+
"'" + clauseClassName +
129+
"' clause-defined `assemblyFormat` not present in operation. "
130+
"Consider concatenating `clausesAssemblyFormat` or explicitly "
131+
"skipping this field.");
132+
133+
if (!clause->getValueAsBit("ignoreDesc") &&
134+
!verifyStringValue("description", op, clause))
135+
PrintError(op->getLoc(),
136+
"'" + clauseClassName +
137+
"' clause-defined `description` not present in operation. "
138+
"Consider concatenating `clausesDescription` or explicitly "
139+
"skipping this field.");
140+
141+
if (!clause->getValueAsBit("ignoreExtraDecl") &&
142+
!verifyStringValue("extraClassDeclaration", op, clause))
143+
PrintWarning(
144+
op->getLoc(),
145+
"'" + clauseClassName +
146+
"' clause-defined `extraClassDeclaration` not present in "
147+
"operation. Consider concatenating `clausesExtraClassDeclaration` "
148+
"or explicitly skipping this field.");
149+
}
150+
151+
/// Verify that all properties of `OpenMP_Clause`s of records deriving from
152+
/// `OpenMP_Op`s have been inherited by the latter.
153+
static bool verifyDecls(const RecordKeeper &recordKeeper, raw_ostream &) {
154+
for (Record *op : recordKeeper.getAllDerivedDefinitions("OpenMP_Op")) {
155+
for (Record *clause : op->getValueAsListOfDefs("clauseList"))
156+
verifyClause(op, clause);
157+
}
158+
159+
return false;
160+
}
161+
162+
// Registers the generator to mlir-tblgen.
163+
static mlir::GenRegistration
164+
verifyOpenmpOps("verify-openmp-ops",
165+
"Verify OpenMP operations (produce no output file)",
166+
verifyDecls);

0 commit comments

Comments
 (0)