[MLIR] Add OpenMP dialect with barrier operation

[DavidTruby]

Summary

Barrier is a simple operation that takes no arguments and returns
nothing, but implies a side effect (synchronization of all threads)

[jdoerfert]

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

12

In other places we use namespace omp, either way it probably makes sense to be consistent.

[DavidTruby]

Renamed openmp namespace to omp

[rriddle]

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

1

All of these files are missing top-level file comments.

6

Why is this header necessary?

7

Is OpImplementation.h necessary here?

26

These should use // style comments. Please refer to other similar files for examples.

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

24

Need an extra space here.

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

4

Only OpenMPDialect.h seems necessary here.

6

Use using namespace ... instead, (with explicit scoping when necessary)

See other files for examples.

9

nit: Use getDialectNamespace instead of hardcoding the string.

19

nit: openMPDialect?

[jdoerfert]

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

19

If we spell out OpenMP it should be OpenMP.

[rriddle]

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

19

MLIR uses camelCase variable names.

[DavidTruby]

Address various review comments

[DavidTruby]

Added top-level file comments.

[DavidTruby]

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

7

I moved this include to the .cpp file. If I don't have it at all I get linker errors related to some missing function definitions.

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

19

I've changed this to ompDialect to be consistent with the namespace name change. Let me know if there's a preferred name for this though.

[jdoerfert]

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

19

MLIR uses camelCase variable names.

While that is interesting on its own, my comment was not targeted at variable naming conventions but rather towards how we commonly, and even in MLIR, treat "names". "OpenMP" is (used as) a proper name, doing anything to it seems problematic, e.g., we cannot properly search for it. We also have ample of precedence, e.g. GPU, LLVM, SPIRV, .... at least I haven't seen gPU, lLVM, or sPIRV.

I've changed this to ompDialect to be consistent with the namespace name change. Let me know if there's a preferred name for this though.

That sounds fine to me.

[rriddle]

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

3

Please format this to one line.

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

28

This is the only thing that should be wrapped in a 'namespace mlir { namespace omp {} }`

[DavidTruby]

Small corrections for review.

[DavidTruby]

gentle ping

[bondhugula]

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

10

Missing period at the end.

[rriddle]

I don't remember that we ever fully resolved this(adding OpenMP Dialect). Can you make a ping on llvm.discourse.group to make sure?

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

4

MLIR -> LLVM now

10

nit: Missing punctuation at the end of the line.

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

4

Same here.

[kiranchandramohan]

In D72400#1840949, @rriddle wrote:

I don't remember that we ever fully resolved this(adding OpenMP Dialect). Can you make a ping on llvm.discourse.group to make sure?

The original RFC was sent to mlir google groups in December. (https://groups.google.com/a/tensorflow.org/d/msg/mlir/SCerbBpoxng/bVqWTRY7BAAJ)
I have now posted this RFC in discourse.
https://llvm.discourse.group/t/rfc-openmp-dialect-in-mlir/397

[rriddle]

Approval modulo no strong comments on discourse and after fixing the last few remaining nits.

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

4

Same here.

[DavidTruby]

I've committed and pushed this after fixing the nits, please revert if I misunderstood or if the discourse discussion decides this doesn't want to be here.
Thanks!

mlir/include/mlir/Dialect/CMakeLists.txt

 add_subdirectory(AffineOps)
 add_subdirectory(FxpMathOps)
 add_subdirectory(GPU)
 add_subdirectory(Linalg)
 add_subdirectory(LLVMIR)
 add_subdirectory(LoopOps)
+add_subdirectory(OpenMP)
 add_subdirectory(QuantOps)
 add_subdirectory(SPIRV)
 add_subdirectory(StandardOps)
 add_subdirectory(VectorOps)

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

+add_mlir_dialect(OpenMPOps OpenMPOps)

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/IR/FunctionSupport.h"
+#include "mlir/IR/OpImplementation.h"
+
+
+namespace mlir {
+namespace omp {
+
+#define GET_OP_CLASSES
+#include "mlir/Dialect/OpenMP/OpenMPOps.h.inc"
+
+class OpenMPDialect : public Dialect {
+public:
+  explicit OpenMPDialect(MLIRContext *context);
+
+  static StringRef getDialectNamespace() { return "omp"; }
+};
+
+} // namespace openmp
+} // 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";
+}
+
+class OpenMP_Op<string mnemonic, list<OpTrait> traits = []> :
+      Op<OpenMP_Dialect, mnemonic, traits>;
+
+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 parser = [{ return success(); }];
+  let printer = [{ p << getOperationName(); }];
+}
+
+#endif //OPENMP_OPS

mlir/lib/Dialect/CMakeLists.txt

 add_subdirectory(AffineOps)
 add_subdirectory(FxpMathOps)
 add_subdirectory(GPU)
 add_subdirectory(Linalg)
 add_subdirectory(LLVMIR)
 add_subdirectory(LoopOps)
+add_subdirectory(OpenMP)
 add_subdirectory(QuantOps)
 add_subdirectory(SDBM)
 add_subdirectory(SPIRV)
 add_subdirectory(StandardOps)
 add_subdirectory(VectorOps)
 
 add_llvm_library(MLIRDialect
   Traits.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect
   )
 target_link_libraries(MLIRDialect MLIRIR)

mlir/lib/Dialect/OpenMP/CMakeLists.txt

+add_llvm_library(MLIROpenMP
+  IR/OpenMPDialect.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${MLIR_MAIN_INCLUDE_DIR}/mlir/Dialect/OpenMP
+  )
+
+add_dependencies(MLIROpenMP MLIROpenMPOpsIncGen)

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

+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/IR/MLIRContext.h"
+#include "mlir/IR/Operation.h"
+#include "mlir/IR/OperationSupport.h"
+
+namespace mlir {
+namespace omp {
+
+OpenMPDialect::OpenMPDialect(MLIRContext *context) : Dialect("omp", context) {
+  addOperations<
+#define GET_OP_LIST
+#include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"
+      >();
+}
+
+#define GET_OP_CLASSES
+#include "mlir/Dialect/OpenMP/OpenMPOps.cpp.inc"
+
+static DialectRegistration<OpenMPDialect> openmpDialect;
+
+} // namespace openmp
+} // 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
+}

mlir/tools/mlir-opt/CMakeLists.txt

set(LIBS
   MLIRGPU
   MLIRGPUtoNVVMTransforms
   MLIRGPUtoROCDLTransforms
   MLIRGPUtoSPIRVTransforms
   MLIRLinalg
   MLIRLLVMIR
   MLIRLoopOps
   MLIRNVVMIR
+  MLIROpenMP
   MLIROptMain
   MLIRParser
   MLIRPass
   MLIRQuantizerTransforms
   MLIRQuantOps
   MLIRROCDLIR
   MLIRSPIRV
   MLIRStandardToSPIRVTransforms