This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] [OpenMP] Add basic OpenMP parallel operation
ClosedPublic

Authored by DavidTruby on May 5 2020, 5:11 AM.

Details

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 <kiran.chandramohan@arm.com>

Diff Detail

Event Timeline

DavidTruby created this revision.May 5 2020, 5:11 AM
Herald added a project: Restricted Project. · View Herald Transcript

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

  1. 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 added inline comments.May 5 2020, 11:50 AM
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 marked an inline comment as done.May 5 2020, 12:28 PM
DavidTruby added inline comments.
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 added inline comments.May 5 2020, 1:08 PM
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 marked an inline comment as done.May 5 2020, 1:32 PM
DavidTruby added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
131

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

jdoerfert added inline comments.May 5 2020, 1:40 PM
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 marked an inline comment as done.May 5 2020, 2:39 PM
DavidTruby added inline comments.
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....

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 marked an inline comment as done.May 6 2020, 8:11 AM
DavidTruby added inline comments.
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 marked an inline comment as done.May 6 2020, 8:23 AM
DavidTruby added inline comments.
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 added inline comments.May 6 2020, 9:15 AM
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 updated this revision to Diff 262405.May 6 2020, 9:47 AM

Address review comments

DavidTruby marked 7 inline comments as done.May 6 2020, 1:08 PM
ftynse requested changes to this revision.May 7 2020, 2:24 AM

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.

This revision now requires changes to proceed.May 7 2020, 2:24 AM
DavidTruby marked an inline comment as done.May 7 2020, 5:43 AM
DavidTruby added inline comments.
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 updated this revision to Diff 262632.May 7 2020, 6:08 AM

Removed custom parser and pretty printer.

ftynse added inline comments.May 9 2020, 2:31 AM
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.

clementval added a project: Restricted Project.May 13 2020, 8:52 AM
clementval added a subscriber: clementval.
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);

  1. 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 added inline comments.May 18 2020, 9:39 AM
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 marked an inline comment as done.

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

ftynse accepted this revision.May 27 2020, 7:33 AM
This revision is now accepted and ready to land.May 27 2020, 7:33 AM

@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 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 :)

This revision was automatically updated to reflect the committed changes.

@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?

@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 added a comment.EditedMay 28 2020, 6:57 AM

@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.