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

[MLIR][OpenMP] NFC: Address tablegen warnings #98485

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

skatrak
Copy link
Contributor

@skatrak skatrak commented Jul 11, 2024

After the addition of the -verify-openmp-ops tablegen pseudo-backend to report suspected issues with the definition of an operation, some warnings were triggered in the current implementation of OpenMPOps.td.

These are related to known limitations for which TODO messages exist within these operation's definitions. This patch explicitly inhibits the inheritance of the assemblyFormat property of all clauses added to the omp.parallel and omp.wsloop operations to avoid these warnings. This should help identifying actual issues that may arise later as the dialect continues to be developed.

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 11, 2024

@llvm/pr-subscribers-flang-openmp
@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-openmp

Author: Sergio Afonso (skatrak)

Changes

After the addition of the -verify-openmp-ops tablegen pseudo-backend to report suspected issues with the definition of an operation, some warnings were triggered in the current implementation of OpenMPOps.td.

These are related to known limitations for which TODO messages exist within these operation's definitions. This patch explicitly inhibits the inheritance of the assemblyFormat property of all clauses added to the omp.parallel and omp.wsloop operations to avoid these warnings. This should help identifying actual issues that may arise later as the dialect continues to be developed.


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

1 Files Affected:

  • (modified) mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td (+16-6)
diff --git a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
index aee2937ce5cb7..69fd1f1f0130f 100644
--- a/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ b/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -133,8 +133,12 @@ def ParallelOp : OpenMP_Op<"parallel", traits = [
     RecursiveMemoryEffects
   ], clauses = [
     // TODO: Sort clauses alphabetically.
-    OpenMP_IfClause, OpenMP_NumThreadsClause, OpenMP_AllocateClause,
-    OpenMP_ReductionClause, OpenMP_ProcBindClause, OpenMP_PrivateClause
+    OpenMP_IfClauseSkip<assemblyFormat = true>,
+    OpenMP_NumThreadsClauseSkip<assemblyFormat = true>,
+    OpenMP_AllocateClauseSkip<assemblyFormat = true>,
+    OpenMP_ReductionClauseSkip<assemblyFormat = true>,
+    OpenMP_ProcBindClauseSkip<assemblyFormat = true>,
+    OpenMP_PrivateClauseSkip<assemblyFormat = true>
   ], singleRegion = true> {
   let summary = "parallel construct";
   let description = [{
@@ -154,7 +158,8 @@ def ParallelOp : OpenMP_Op<"parallel", traits = [
 
   // TODO: Use default assembly format inherited from OpenMP_Op once printing
   // and parsing of the parallel region is not intermingled with printing and
-  // parsing of reduction and private clauses.
+  // parsing of reduction and private clauses. `assemblyFormat` should also be
+  // no longer skipped for clauses added to this operation at that time.
   let assemblyFormat = [{
     oilist(
           `if` `(` $if_expr `)`
@@ -363,8 +368,12 @@ def WsloopOp : OpenMP_Op<"wsloop", traits = [
   ], clauses = [
     // TODO: Complete clause list (allocate, private).
     // TODO: Sort clauses alphabetically.
-    OpenMP_LinearClause, OpenMP_ReductionClause, OpenMP_ScheduleClause,
-    OpenMP_NowaitClause, OpenMP_OrderedClause, OpenMP_OrderClause
+    OpenMP_LinearClauseSkip<assemblyFormat = true>,
+    OpenMP_ReductionClauseSkip<assemblyFormat = true>,
+    OpenMP_ScheduleClauseSkip<assemblyFormat = true>,
+    OpenMP_NowaitClauseSkip<assemblyFormat = true>,
+    OpenMP_OrderedClauseSkip<assemblyFormat = true>,
+    OpenMP_OrderClauseSkip<assemblyFormat = true>
   ], singleRegion = true> {
   let summary = "worksharing-loop construct";
   let description = [{
@@ -398,7 +407,8 @@ def WsloopOp : OpenMP_Op<"wsloop", traits = [
 
   // TODO: Use default assembly format inherited from OpenMP_Op once printing
   // and parsing of the workshare loop region is not intermingled with printing
-  // and parsing of reduction clauses.
+  // and parsing of reduction clauses. `assemblyFormat` should also be no longer
+  // skipped for clauses added to this operation at that time.
   let assemblyFormat = [{
     oilist(`linear` `(`
               custom<LinearClause>($linear_vars, type($linear_vars),

Copy link
Contributor

@mjklemm mjklemm left a comment

Choose a reason for hiding this comment

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

LGTM

@anchuraj
Copy link
Contributor

LGTM

After the addition of the `-verify-openmp-ops` tablegen pseudo-backend to
report suspected issues with the definition of an operation, some warnings were
triggered in the current implementation of OpenMPOps.td.

These are related to known limitations for which TODO messages exist within
these operation's definitions. This patch explicitly inhibits the inheritance
of the `assemblyFormat` property of all clauses added to the `omp.parallel` and
`omp.wsloop` operations to avoid these warnings. This should help identifying
actual issues that may arise later as the dialect continues to be developed.
@skatrak skatrak merged commit 03d8f95 into llvm:main Jul 15, 2024
5 of 6 checks passed
@skatrak skatrak deleted the omp-tblgen-warnings branch July 15, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants