[PATCH] D72400: [MLIR] Add OpenMP dialect with barrier operation

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h:1
+#ifndef MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_
+#define MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_
----------------
All of these files are missing top-level file comments.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h:6
+#include "mlir/IR/OpDefinition.h"
+#include "mlir/IR/FunctionSupport.h"
+#include "mlir/IR/OpImplementation.h"
----------------
Why is this header necessary?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h:7
+#include "mlir/IR/FunctionSupport.h"
+#include "mlir/IR/OpImplementation.h"
+
----------------
Is OpImplementation.h necessary here?


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h:26
+
+#endif /* MLIR_DIALECT_OPENMP_OPENMPDIALECT_H_ */
----------------
These should use `// ` style comments. Please refer to other similar files for examples.


================
Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:24
+
+#endif //OPENMP_OPS
----------------
Need an extra space here.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:4
+#include "mlir/IR/Operation.h"
+#include "mlir/IR/OperationSupport.h"
+
----------------
Only OpenMPDialect.h seems necessary here.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:6
+
+namespace mlir {
+namespace omp {
----------------
Use `using namespace ...` instead, (with explicit scoping when necessary)

See other files for examples.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:9
+
+OpenMPDialect::OpenMPDialect(MLIRContext *context) : Dialect("omp", context) {
+  addOperations<
----------------
nit: Use `getDialectNamespace` instead of hardcoding the string.


================
Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:19
+
+static DialectRegistration<OpenMPDialect> openmpDialect;
+
----------------
nit: openMPDialect?


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

https://reviews.llvm.org/D72400