[PATCH] D79410: [MLIR] [OpenMP] Add basic OpenMP parallel operation

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added a comment.
This revision now requires changes to proceed.

The parser and printer seem broken, and not exercised by tests. If you only need the op itself, you may start by implementing it and omit the custom printer/parser while relying on the generic syntax.



================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:19
 
+
 def OpenMP_Dialect : Dialect {
----------------
Nit: spurious newline


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:67
+  let arguments = (ins Optional<I1>:$if_expr_var,
+             Optional<AnySignedInteger>:$num_threads_var,
+             OptionalAttr<ClauseDefault>:$default_val,
----------------
Do we care about integer being signed for num_threads?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:81
+def TerminatorOp : OpenMP_Op<"terminator", [Terminator]>,
+     Results<(outs)> {
+  let summary = "terminator for OpenMP regions.";
----------------
You can just omit the `Results` class, `(outs)` is the default value for results.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:37
+  // Parse operand list
+  if (parser.parseOperandList(operands))
+    return failure();
----------------
How would you differentiate between operands that are private/firstprivate/... ? 


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:49
+  Type type;
+  if (parser.parseColon() || parser.parseColonType(type))
+    return failure();
----------------
This still parsers the colon twice: first time in parseColon, second time in parseColonType


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:61
+  llvm::interleaveComma(op.getOperands(), p, [&](auto value) {
+    p << value << " : " << value.getType();
+  });
----------------
This contradicts the parser, which expects a plain comma-separated list of values, and prints a parenthesized list of values with types.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:67
+                                     {SymbolTable::getSymbolAttrName()});
+  p.printRegion(op.getOperation()->getRegion(0), /*printEntryBlockArgs=*/false,
+                /*printBlockTerminators=*/false);
----------------
Replace `op.getOperation()->getRegion(0)` with `op.region()`, we generate accessors for regions named in ODS files


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:69
+                /*printBlockTerminators=*/false);
+}
+
----------------
The parser expects a trailing _single_ type...


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:1
 // RUN: mlir-opt -verify-diagnostics %s | FileCheck %s
 
----------------
This needs a check that you can _parse back_ what you printed, i.e. `mlir-opt %s | mlir-opt | FileCheck %s`.

Also, drop `verify-diagnostics`, there are no diagnostics in this file. Generally, we prefer diagnostic tests to be in a separate file.


================
Comment at: mlir/test/Dialect/OpenMP/ops.mlir:28
+  // CHECK: omp_parallel
+  "omp.parallel" (%if_cond, %num_threads, %data_var, %data_var, %data_var, %data_var) ({
+
----------------
These tests only use the generic syntax for `omp.parallel` so they don't exercise the custom parser you wrote. Generally, we trust the core infra tests to exercise the generic syntax and don't write tests for every op in the generic syntax. We do use generic syntax to test custom verifiers in cases when they cannot be triggered using the custom parser.

We do however write tests for the custom non-generated syntax, which you did not provide here. From reading the code in this diff, I suspect the parser and printer for `omp.parallel` dont match and there are no tests to convince me otherwise.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79410/new/

https://reviews.llvm.org/D79410