Page MenuHomePhabricator

[MLIR] Add OpenMP dialect with barrier operation
ClosedPublic

Authored by DavidTruby on Jan 8 2020, 7:11 AM.

Details

Summary

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

Diff Detail

Event Timeline

DavidTruby created this revision.Jan 8 2020, 7:11 AM
Herald added a project: Restricted Project. · View Herald Transcript
DavidTruby retitled this revision from Add OpenMP dialect with barrier operation to [MLIR] Add OpenMP dialect with barrier operation.
jdoerfert added inline comments.Jan 8 2020, 7:55 AM
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 updated this revision to Diff 236832.Jan 8 2020, 8:15 AM

Renamed openmp namespace to omp

DavidTruby marked an inline comment as done.Jan 8 2020, 8:15 AM
rriddle requested changes to this revision.Jan 8 2020, 10:46 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
2

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

7

Why is this header necessary?

8

Is OpImplementation.h necessary here?

27

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

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

Need an extra space here.

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

Only OpenMPDialect.h seems necessary here.

7

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

See other files for examples.

10

nit: Use getDialectNamespace instead of hardcoding the string.

20

nit: openMPDialect?

This revision now requires changes to proceed.Jan 8 2020, 10:46 AM
jdoerfert added inline comments.Jan 8 2020, 11:28 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
20

If we spell out OpenMP it should be OpenMP.

rriddle added inline comments.Jan 8 2020, 11:37 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
20

MLIR uses camelCase variable names.

DavidTruby updated this revision to Diff 236987.Jan 9 2020, 2:16 AM
DavidTruby marked an inline comment as done.

Address various review comments

DavidTruby updated this revision to Diff 236992.Jan 9 2020, 2:25 AM

Added top-level file comments.

DavidTruby marked 8 inline comments as done.Jan 9 2020, 2:27 AM
DavidTruby added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPDialect.h
8

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
20

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.

DavidTruby marked an inline comment as done.Jan 9 2020, 2:41 AM
jdoerfert added inline comments.Jan 9 2020, 8:40 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
20

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 added inline comments.Jan 9 2020, 10:37 AM
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 {} }`

Small corrections for review.

DavidTruby marked 2 inline comments as done.Jan 13 2020, 4:07 AM
bondhugula accepted this revision.Jan 26 2020, 7:48 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
10

Missing period at the end.

rriddle requested changes to this revision.Jan 26 2020, 8:26 AM

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.

This revision now requires changes to proceed.Jan 26 2020, 8:26 AM

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 accepted this revision.Jan 28 2020, 4:22 PM

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.

This revision is now accepted and ready to land.Jan 28 2020, 4:22 PM
This revision was automatically updated to reflect the committed changes.

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!