[MLIR] [OpenMP] Add basic OpenMP parallel operation

[DavidTruby]

Summary

This includes a basic implementation for the OpenMP parallel
operation with only a very simple pretty-printer and custom parser.
The if, num_threads, private, shared, first_private, last_private,
proc_bind and default clauses are included in this implementation.

Currently the reduction clause is omitted as it is more complex and
requires analysis to see if we can share implementation with the loop
dialect.

Co-authored-by: Kiran Chandramohan <[e-mail]>

[kiranchandramohan]

Can you add a pointers to

1. the discourse discussion for context in the summary?

https://llvm.discourse.group/t/openmp-parallel-operation-design-issues/686

2. the openmp spec ( I think the link above has a pointer to it). Also say allocate from 5.0 is not there.

Some comments inline.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

153

summary starts with small letter for other constructs.

157–158

Is the terminator Op required if the behaviour is just to return control to enclosing op? Hopefully the reviewers can help here.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

74

Remove this?

mlir/test/Dialect/OpenMP/ops.mlir

26

For this test can we,
-> Add empty lines in between various checks for readability.
-> If the arguments are named to match what they mean it will be easier to read. (I agree that it can also mislead. :))

32

Can this be removed or made optional?

[rriddle]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

108

nit: Can you add more documentation for what these correspond to?

131

Is there a logical ordering of some kind you can establish for this file, e.g., alphabetical?

132

nit: Can you use let arguments = (ins ...) instead? The op definitions end up being much cleaner IMO.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

49

nit: parseColonType

59

Can you use llvm::interleaveComma instead?

[DavidTruby]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

131

The order they're in here is the order they're in in the OpenMP specification. I can change it to be alphabetical or otherwise though

[rriddle]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

131

That makes sense, it wasn't clear that there was an order on first glance. Can you add comment blocks to make this a bit cleaner?

e.g.: https://github.com/llvm/llvm-project/blob/4e9a7c8f5c527e7493394ab7869f38ca7c6b8903/mlir/include/mlir/Dialect/StandardOps/IR/Ops.td#L205

[DavidTruby]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

131

Sure, I've realised that I should really add more comments to this in general!

[jdoerfert]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

111

If the preprocessor runs on this file you can use the OMPKinds.def macros to avoid listing stuff explicitly here. That also keeps everything in sync.

[DavidTruby]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

111

I'll be honest, I have absolutely no idea how tablegen works. Will the preprocessor be run on files generated by this? I assume it will but I don't really know....

[kiranchandramohan]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

108

These are all values for the default clause : private, firstprivate, shared, none. Had to add the 'def' prefix since there was some error in the code generated from the spec because private is a keyword in c++.
Is there another way to solve this and keep the clause values as private rather than defprivate?

111

I am guessing it will run on this file because of the presence of include and #define at the top of this file (lines 14-17).

[DavidTruby]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

111

I tried this and it seems like tablegen tries to interpret #define and then shouts at me. Any ideas how I can get it to ignore my #define and just put it in the generated code?

[DavidTruby]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

111

Doesn't look like this is possible due to tablegen having its own custom preprocessor unfortunately. One suggestion I've seen is to make that header be generated by tablegen so that we can use it in tablegen files. Given that this will probably come up again in other constructs that might be worth us doing, but I think it should be done as a separate patch. What do you think @jdoerfert ?

[jdoerfert]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

111

1. A separate patch is generally fine.
2. We should investigate reuse opportunities now, before we duplicate tings ;)
3. We could probably preprocess a file explicitly and include it here *or* move OMPKinds.def to a tablegen file as well (part by part). I did avoid that so far but it might make sense in the long run.

[DavidTruby]

Address review comments

[ftynse]

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.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

19

Nit: spurious newline

67

Do we care about integer being signed for num_threads?

81

You can just omit the Results class, (outs) is the default value for results.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

37

How would you differentiate between operands that are private/firstprivate/... ?

49

This still parsers the colon twice: first time in parseColon, second time in parseColonType

61

This contradicts the parser, which expects a plain comma-separated list of values, and prints a parenthesized list of values with types.

67

Replace op.getOperation()->getRegion(0) with op.region(), we generate accessors for regions named in ODS files

69

The parser expects a trailing _single_ type...

mlir/test/Dialect/OpenMP/ops.mlir

1

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.

28

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.

[DavidTruby]

mlir/test/Dialect/OpenMP/ops.mlir

28

I'm working on a parser and pretty printer separately, so I think I should just remove the ones here. I wasn't sure if I still needed them or not in the mean time but from your comments it seems not. I'll remove them.

[DavidTruby]

Removed custom parser and pretty printer.

[ftynse]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

67

Could you please elaborate why you need a _signed_ integer here, and not just AnyInteger. And what is the semantics of passing a negative value here.

Generally, given the number of attributes to this op, it really needs a description field.

mlir/test/Dialect/OpenMP/ops.mlir

28

If you have them, the _will_ be used, but the tests don't exercise them.

[kiranchandramohan]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

67

1. I think it is OK to be AnyInteger. Is your point that if we only allow it to be unsigned then a conversion is needed to create this operation if the value of num_integer is stored in an unsigned variable?

I am providing some relevant information, please advise whether AnyInteger is the most suitable.

If a negative value is passed, it should be an error. But the standard allows for it to be an expression which can evaluate to a positive value.

The OpenMP standard has the following specification and restriction for num_threads.
num_threads(scalar-integer-expression)
The num_threads expression must evaluate to a positive integer value.

The related omp_set_num_threads OpenMP API call in C takes in a signed integer
void omp_set_num_threads(int num_threads);

2. The other related point is that the frontend might have evaluated this to a value already. If that is always the case then num_threads can be an attribute. But I am assuming that the mlir operation should be generic enough to allow what is described in the standard (scalar-integer-expression which evaluates to a positive value).

[ftynse]

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

67

I think involving the signed/unsigned/signless difference is adding unnecessary complexity here. This is not fixing the bitwidth of the integer either... Also, most operations in the standard dialect, which are most likely to be used to implement the expression, operate on signless integers so extra work will be necessary to pass the expression result to an operation that only accepts signed integers.

MLIR lets you declare a constant value via std.constant. So if a frontend has folded num_threads into a constant, it can just emit one more operation and use the omp.parallel version expecting a value.

Please add a description field to this operation and let's get this landed.

[DavidTruby]

Extended the description field to include more documentation of the
arguments to the operation, and changed num_threads to accept AnyInteger.

[DavidTruby]

@jdoerfert I'm looking into tablegen-ifying the sections of OMPKinds.def that I am duplicating here, would you be ok with me submitting that later as a separate patch as there are other patches waiting on this work? Or do you want me to make sure that's done first before merging this?

[jdoerfert]

In D79410#2057389, @DavidTruby wrote:

@jdoerfert I'm looking into tablegen-ifying the sections of OMPKinds.def that I am duplicating here, would you be ok with me submitting that later as a separate patch as there are other patches waiting on this work? Or do you want me to make sure that's done first before merging this?

We can do this when you see fit. I think it will help us in the long run and I wanted to make sure we consider it early to avoid work creating duplicate content :)

[clementval]

In D79410#2057389, @DavidTruby wrote:

@jdoerfert I'm looking into tablegen-ifying the sections of OMPKinds.def that I am duplicating here, would you be ok with me submitting that later as a separate patch as there are other patches waiting on this work? Or do you want me to make sure that's done first before merging this?

@DavidTruby I'm pretty intersted about the tablegen way of doing the OMPKinds.def since I'm working on this for OpenACC. Do you have any early things I can have a look at and maybe give some help as well?

[DavidTruby]

In D79410#2058407, @clementval wrote:

In D79410#2057389, @DavidTruby wrote:

@jdoerfert I'm looking into tablegen-ifying the sections of OMPKinds.def that I am duplicating here, would you be ok with me submitting that later as a separate patch as there are other patches waiting on this work? Or do you want me to make sure that's done first before merging this?

@DavidTruby I'm pretty intersted about the tablegen way of doing the OMPKinds.def since I'm working on this for OpenACC. Do you have any early things I can have a look at and maybe give some help as well?

@clementval I'm really just trying to get my head around tablegen at the moment so I don't have much to show yet. Is there some way I can communicate with you better than on phabricator comments so I can share with you what I've got when I've got my head around it more?

[clementval]

In D79410#2059904, @DavidTruby wrote:

In D79410#2058407, @clementval wrote:

In D79410#2057389, @DavidTruby wrote:

@jdoerfert I'm looking into tablegen-ifying the sections of OMPKinds.def that I am duplicating here, would you be ok with me submitting that later as a separate patch as there are other patches waiting on this work? Or do you want me to make sure that's done first before merging this?

@DavidTruby I'm pretty intersted about the tablegen way of doing the OMPKinds.def since I'm working on this for OpenACC. Do you have any early things I can have a look at and maybe give some help as well?

@clementval I'm really just trying to get my head around tablegen at the moment so I don't have much to show yet. Is there some way I can communicate with you better than on phabricator comments so I can share with you what I've got when I've got my head around it more?

@DavidTruby
Sure, you can contact me by email: clementv at ornl dot gov
I'm also on the Flang slack channel. My username is clementval as well.

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

-add_mlir_dialect(OpenMPOps omp)
+set(LLVM_TARGET_DEFINITIONS OpenMPOps.td)
+mlir_tablegen(OpenMPOpsDialect.h.inc -gen-dialect-decls -dialect=omp)
+mlir_tablegen(OpenMPOps.h.inc -gen-op-decls)
+mlir_tablegen(OpenMPOps.cpp.inc -gen-op-defs)
+mlir_tablegen(OpenMPOpsEnums.h.inc -gen-enum-decls)
+mlir_tablegen(OpenMPOpsEnums.cpp.inc -gen-enum-defs)
 add_mlir_doc(OpenMPOps -gen-dialect-doc OpenMPDialect Dialects/)
+add_public_tablegen_target(MLIROpenMPOpsIncGen)

mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h

 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_
 #define MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_
 
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/OpDefinition.h"
 
+#include "mlir/Dialect/OpenMP/OpenMPOpsEnums.h.inc"
+
 namespace mlir {
 namespace omp {
 
 #define GET_OP_CLASSES
 #include "mlir/Dialect/OpenMP/OpenMPOps.h.inc"
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.h.inc"
 } // namespace omp
 } // namespace mlir
 
 #endif // MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td

 //===----------------------------------------------------------------------===//
 
 
 #ifndef OPENMP_OPS
 #define OPENMP_OPS
 
 include "mlir/IR/OpBase.td"
 
+
 def OpenMP_Dialect : Dialect {
   let name = "omp";
+  let cppNamespace = "omp";
 }
 
 class OpenMP_Op<string mnemonic, list<OpTrait> traits = []> :
       Op<OpenMP_Dialect, mnemonic, traits>;
 
-def BarrierOp : OpenMP_Op<"barrier"> {
-  let summary = "barrier construct";
+
+//===----------------------------------------------------------------------===//
+// 2.6 parallel Construct
+//===----------------------------------------------------------------------===//
+
+// Possible values for the default clause
+def ClauseDefaultPrivate : StrEnumAttrCase<"defprivate">;
+def ClauseDefaultFirstPrivate : StrEnumAttrCase<"deffirstprivate">;
+def ClauseDefaultShared : StrEnumAttrCase<"defshared">;
+def ClauseDefaultNone : StrEnumAttrCase<"defnone">;
+
+def ClauseDefault : StrEnumAttr<
+    "ClauseDefault",
+    "default clause",
+    [ClauseDefaultPrivate, ClauseDefaultFirstPrivate, ClauseDefaultShared,
+     ClauseDefaultNone]> {
+  let cppNamespace = "::mlir::omp";
+}
+
+// Possible values for the proc_bind clause
+def ClauseProcMaster : StrEnumAttrCase<"master">;
+def ClauseProcClose : StrEnumAttrCase<"close">;
+def ClauseProcSpread : StrEnumAttrCase<"spread">;
+
+def ClauseProcBind : StrEnumAttr<
+    "ClauseProcBind",
+    "procbind clause",
+    [ClauseProcMaster, ClauseProcClose, ClauseProcSpread]> {
+  let cppNamespace = "::mlir::omp";
+}
+
+def ParallelOp : OpenMP_Op<"parallel", [AttrSizedOperandSegments]> {
+  let summary = "parallel construct";
   let description = [{
-    The barrier construct specifies an explicit barrier at the point at which
-    the construct appears.
+    The parallel construct includes a region of code which is to be executed
+    by a team of threads.
   }];
 
-  let assemblyFormat = "attr-dict";
+  let arguments = (ins Optional<I1>:$if_expr_var,
+             Optional<AnySignedInteger>:$num_threads_var,
+             OptionalAttr<ClauseDefault>:$default_val,
+             Variadic<AnyType>:$private_vars,
+             Variadic<AnyType>:$firstprivate_vars,
+             Variadic<AnyType>:$shared_vars,
+             Variadic<AnyType>:$copyin_vars,
+             OptionalAttr<ClauseProcBind>:$proc_bind_val);
+
+  let regions = (region AnyRegion:$region);
+  let parser = [{ return parseParallelOp(parser, result); }];
+  let printer = [{ printParallelOp(p, *this); }];
 }
 
-def TaskwaitOp : OpenMP_Op<"taskwait"> {
-  let summary = "taskwait construct";
+def TerminatorOp : OpenMP_Op<"terminator", [Terminator]>,
+     Results<(outs)> {
+  let summary = "terminator for OpenMP regions.";
   let description = [{
-    The taskwait construct specifies a wait on the completion of child tasks
-    of the current task.
+    A terminator operation for regions that appear in the body of OpenMP
+    operation.  These regions are not expected to return any value so the
+    terminator takes no operands. The terminator op returns control to the
+    enclosing op.
   }];
 
-  let assemblyFormat = "attr-dict";
+  let parser = [{ return success(); }];
+  let printer = [{ p << getOperationName(); }];
 }
 
+//===----------------------------------------------------------------------===//
+// 2.10.4 taskyield Construct
+//===----------------------------------------------------------------------===//
+
 def TaskyieldOp : OpenMP_Op<"taskyield"> {
   let summary = "taskyield construct";
   let description = [{
     The taskyield construct specifies that the current task can be suspended
     in favor of execution of a different task.
   }];
 
   let assemblyFormat = "attr-dict";
 }
 
+//===----------------------------------------------------------------------===//
+// 2.17.2 barrier Construct
+//===----------------------------------------------------------------------===//
+
+def BarrierOp : OpenMP_Op<"barrier"> {
+  let summary = "barrier construct";
+  let description = [{
+    The barrier construct specifies an explicit barrier at the point at which
+    the construct appears.
+  }];
+
+  let assemblyFormat = "attr-dict";
+}
+
+//===----------------------------------------------------------------------===//
+// 2.17.5 taskwait Construct
+//===----------------------------------------------------------------------===//
+
+def TaskwaitOp : OpenMP_Op<"taskwait"> {
+  let summary = "taskwait construct";
+  let description = [{
+    The taskwait construct specifies a wait on the completion of child tasks
+    of the current task.
+  }];
+
+  let assemblyFormat = "attr-dict";
+}
+
 #endif // OPENMP_OPS

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp

 //===- OpenMPDialect.cpp - MLIR Dialect for OpenMP implementation ---------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the OpenMP dialect and its operations.
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/Dialect/StandardOps/IR/Ops.h"
 #include "mlir/IR/OpImplementation.h"
 
+#include "llvm/ADT/StringSwitch.h"
+
+#include "mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc"
+
 using namespace mlir;
 using namespace mlir::omp;
 
 OpenMPDialect::OpenMPDialect(MLIRContext *context)
     : Dialect(getDialectNamespace(), context) {
   addOperations<
 #define GET_OP_LIST
 #include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"
       >();
 }
 
+static ParseResult parseParallelOp(OpAsmParser &parser,
+                                   OperationState &result) {
+  SmallVector<OpAsmParser::OperandType, 8> operands;
+
+  // Parse operand list
+  if (parser.parseOperandList(operands))
+    return failure();
+  // If attributes are present, parse them.
+  if (parser.parseOptionalAttrDictWithKeyword(result.attributes))
+    return failure();
+
+  // Parse the module body.
+  auto *body = result.addRegion();
+  if (parser.parseRegion(*body, None, None))
+    return failure();
+
+  Type type;
+  if (parser.parseColon() || parser.parseColonType(type))
+    return failure();
+  result.addTypes(type);
+
+  return success();
+}
+
+static void printParallelOp(OpAsmPrinter &p, ParallelOp op) {
+  p << op.getOperationName() << ' ';
+  p << '(';
+
+  llvm::interleaveComma(op.getOperands(), p, [&](auto value) {
+    p << value << " : " << value.getType();
+  });
+
+  p << ')';
+  p.printOptionalAttrDictWithKeyword(op.getAttrs(),
+                                     {SymbolTable::getSymbolAttrName()});
+  p.printRegion(op.getOperation()->getRegion(0), /*printEntryBlockArgs=*/false,
+                /*printBlockTerminators=*/false);
+}
+
 namespace mlir {
 namespace omp {
 #define GET_OP_CLASSES
 #include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"
 } // namespace omp
 } // namespace mlir

mlir/test/Dialect/OpenMP/ops.mlir

 // RUN: mlir-opt -verify-diagnostics %s | FileCheck %s
 
 func @omp_barrier() -> () {
   // CHECK: omp.barrier
   omp.barrier
   return
 }
 
 func @omp_taskwait() -> () {
   // CHECK: omp.taskwait
   omp.taskwait
   return
 }
 
 func @omp_taskyield() -> () {
   // CHECK: omp.taskyield
   omp.taskyield
   return
 }
+
+func @omp_terminator() -> () {
+  // CHECK: omp.terminator
+  omp.terminator
+}
+
+func @omp_parallel(%data_var : memref<i32>, %if_cond : i1, %num_threads : si32) -> () {
+  // CHECK: omp_parallel
+  "omp.parallel" (%if_cond, %num_threads, %data_var, %data_var, %data_var, %data_var) ({
+
+  // test without if condition
+  // CHECK: omp.parallel
+    "omp.parallel"(%num_threads, %data_var, %data_var, %data_var, %data_var) ({
+      omp.terminator
+    }) {operand_segment_sizes = dense<[0,1,1,1,1,1]>: vector<6xi32>, default_val = "defshared"} : (si32, memref<i32>, memref<i32>, memref<i32>, memref<i32>) -> ()
+
+  // CHECK: omp.barrier
+    omp.barrier
+
+  // test without num_threads
+  // CHECK: omp.parallel
+    "omp.parallel"(%if_cond, %data_var, %data_var, %data_var, %data_var) ({
+      omp.terminator
+    }) {operand_segment_sizes = dense<[1,0,1,1,1,1]> : vector<6xi32>} : (i1, memref<i32>, memref<i32>, memref<i32>, memref<i32>) -> ()
+
+    omp.terminator
+  }) {operand_segment_sizes = dense<[1,1,1,1,1,1]> : vector<6xi32>, proc_bind_val = "spread"} : (i1, si32, memref<i32>, memref<i32>, memref<i32>, memref<i32>) -> ()
+
+  // test with multiple parameters for single variadic argument
+  // CHECK: omp.parallel
+  "omp.parallel" (%data_var, %data_var, %data_var, %data_var, %data_var) ({
+    omp.terminator
+  }) {operand_segment_sizes = dense<[0,0,1,2,1,1]> : vector<6xi32>} : (memref<i32>, memref<i32>, memref<i32>, memref<i32>, memref<i32>) -> ()
+
+  return
+}