This is an archive of the discontinued LLVM Phabricator instance.

[mlir][openacc] Introduce OpenACC dialect with parallel, data, loop operations
ClosedPublic

Authored by clementval on Jul 21 2020, 1:25 PM.

Details

Summary

This patch introduces the OpenACC dialect with three operation defined
parallel, data and loop operations with custom parsing and printing.

OpenACC dialect RFC can be find here: https://llvm.discourse.group/t/rfc-openacc-dialect/546/2

Diff Detail

Event Timeline

clementval created this revision.Jul 21 2020, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 1:25 PM

Fix some formatting issues

rriddle added inline comments.Jul 23 2020, 11:40 AM
mlir/include/mlir/Dialect/OpenACC/OpenACC.h
14

nit: Please fix this lint error.

30

nit: Please add documentation here.

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
18

nit: Drop the extra newline.

35

Same here.

68

nit: You could invoke the ODS generated accessor here(region()), or just remove this method in favor of that.

150

nit: Other dialects generally put this in the base Op class:

let printer = [{ return ::print(p, *this); }];
let verifier = [{ return ::verify(*this); }];
let parser = [{ return ::parse$cppClass(parser, result); }];

The assemblyFormat overrides the parser/printer fields when present, and if an op doesn't need a verifier you can use let verifier = ?;

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
43

nit: Drop trivial braces, also unsigned int -> unsigned in several places.

47

nit: Can you spell out auto here.

59

nit: Replace this method with parser.parseOptionalAttrDictWithKeyword, it does this for you.

76

nit: Please use proper punctuation in comments.

94

nit: parser.resolveOperands?

132

nit: Please use /// for top-level comments.

255

nit: Spell out auto here.

258

nit: Please add /*paramName=*/ comments to these constants.

471

You can add ParentOneOf to the op definition to capture this constraint.

475

nit: Please avoid the use of e as an iterator variable that isn't end, it can be confusing.

Thanks, added some initial comments.

clementval marked 16 inline comments as done.

Address comments

@rriddle Thanks for the feedback. Just pushed a new version of the patch.

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
150

This is now defined in the base class. I added let verifier = ?; on ops for now until they are implemented in a follow-up patch.

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
63

I am probably missing a point here. (For reference, I have posted below all the clauses for the parallel construct for Open ACC 3.0.)

  1. Where are the allowed attributes specified?
  2. Are only a subset of the clauses modelled here?

async [( int-expr )]
wait [( int-expr-list )]
num_gangs( int-expr )
num_workers( int-expr )
vector_length( int-expr )
device_type( device-type-list )
if( condition )
self [( condition )]
reduction( operator:var-list )
copy( var-list )
copyin( [readonly:]var-list )
copyout( [zero:]var-list )
create( [zero:]var-list )
no_create( var-list )
present( var-list )
deviceptr( var-list )
attach( var-list )
private( var-list )
firstprivate( var-list )
default( none | present )

clementval marked 2 inline comments as done.

Add some "clauses" to parallel op and loop op

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
63

The operation does not have currently all the clauses that are normally allowed. I have updated the patch to add most of them but some will come later when we will implement them such as device_type.
ParallelOp has just a single attribute now that is the reduction operation.

Fix header guard

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
106

Will this preclude code with control flow? Or is this because the standard does not allow such kind of code in OpenACC parallel regions?

clementval marked an inline comment as done.Jul 29 2020, 4:50 PM
clementval marked an inline comment as done.

Update region for parallel, loop and data op

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
106

Actually this is an old copy-paste from my old branch. I just updated the patch so the ops accept AnyRegion similar to GPU func op or omp.parallel op.

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
106

Thanks for making the change.

Actually this is a for my information question also. I am interested in knowing whether having anyregion causes any problems when lowering (particularly the loop) to GPU or linalg dialects?

SouraVX added inline comments.Aug 1 2020, 9:55 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
74

Could you please provide motivation/rationale for this Op having a single region with a single block ?

clementval added inline comments.Aug 1 2020, 10:05 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
74

This is not limited to a single region. But with the SingleBlockImplicitTerminator it limits to 0 or 1 block per region and this is not what is intended. I'll update the ops to accpet more than a single block in the region.

106

I'm not seeing any problem on having 1 or more region as I mentioned during our call.

SouraVX added inline comments.Aug 1 2020, 10:31 AM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
179

Probably I'm missing the bigger picture here, but can't we gave a generic terminator ? This by naming seems specific to DataEndOP ? I

clementval updated this revision to Diff 282480.Aug 2 2020, 2:28 PM

Address review comments

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
179

Good point. Since there is only this op right now that need this default terminator I named it after the op but a generic one would be a better fit. Updated in the patch right now.

SouraVX added inline comments.Aug 3 2020, 12:01 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
179

Didn't noticed the test, for this op. In 'ops.mlir'. Is it too obvious ? Too ignore?

mlir/test/Dialect/OpenMP/ops.mlir
109 ↗(On Diff #282480)

I believe, this change must be unintended.
?
Please take care of it whenever you revise.

SouraVX added inline comments.Aug 3 2020, 12:24 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
177

Is this "_" in naming of Op intentional ? I tried your patch locally, It's resulting in name as acc._terminator. That's a bit odd (leading underscore)considering naming of the rest of the ops in dialect.
This is also why I mentioned for the test case for this Op too. test case helps to capture these in very early stage.

clementval updated this revision to Diff 282772.Aug 3 2020, 5:08 PM
clementval marked 6 inline comments as done.

Address review comments

Updated the patch

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
177

I don't remember which other dialect is using this naming style but I'm fine with removing the underscore from the name.

179

Added in the test,

mlir/test/Dialect/OpenMP/ops.mlir
109 ↗(On Diff #282480)

Sure this is unintended.

LGTM(at least the part I reviewed). But it would be good to wait for @kiranchandramohan and IIRC @ftynse has contributed a lot in GPU dialect. So it would be nice to have there views/thoughts also.
Thanks for your patience :)

LGTM(at least the part I reviewed). But it would be good to wait for @kiranchandramohan and IIRC @ftynse has contributed a lot in GPU dialect. So it would be nice to have there views/thoughts also.
Thanks for your patience :)

Sure will wait on their review. It's the first bits of the dialect so there is no problem to wait some more.

rriddle added inline comments.Aug 4 2020, 10:35 AM
mlir/include/mlir/Dialect/OpenACC/OpenACC.h
30

Top Level comments should use ///

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
80

Please follow the same format as the other dialects for mlir snippets, i.e.

Example:

<triple-tick-here>mlir
<no_indent_snippet_here>
<triple-tick-here>
201

Missing a period here?

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
92

Use llvm::enumerate instead of a separate index.

95

nit: Spell out auto here.

95

nit: Use llvm::interleaveComma here instead of an indexed loop

142

nit: Just move this to a new line instead of trailing the previous.

537

I don't think this is used anywhere.

579

This isn't actually necessary.

This looks ready. A few nit comments.

mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
234

Nit: space after full stop.

238

Is a custom parser/printer not needed since there is only one variadic operand?
Noticed that there are no tests for yield with operand.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
126

Nit: not in sync with contents.

478

Nit: Do you want to specify reduction here?

clementval marked 13 inline comments as done.

Address review comments

clementval added inline comments.Aug 4 2020, 6:48 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
238

Yeah since there is only one variadic operand the op is not flagged as AttrSizedOperandSegments. The assemblyFormat is fine to generate the parser/printer.

It is hard to put together a test using yield with operands without the full reduction being defined, So it will come together I guess in a follow-up patch.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
478

Yes! good catch. Added woker well.

rriddle added inline comments.Aug 4 2020, 6:51 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
238

Yeah since there is only one variadic operand the op is not flagged as AttrSizedOperandSegments. The assemblyFormat is fine to generate the parser/printer.

The assemblyFormat can already handle AttrSizedOperandSegments BTW.

clementval added inline comments.Aug 4 2020, 6:53 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
238

Good to know. Do you have an example of op using this? Would be interested to check how it works.

rriddle added inline comments.Aug 4 2020, 7:06 PM
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
238

It gets handled automatically, you don't have to do anything. E.g. CondBranchOp:
https://github.com/llvm/llvm-project/blob/823ffef009152ba1210740c44d472d1d6e56afa3/mlir/include/mlir/Dialect/StandardOps/IR/Ops.td#L1191

If you use the operands directive instead of listing each operand group individually, the segment size attribute just gets printed along with the attribute dictionary.

clementval marked an inline comment as done.Aug 5 2020, 7:10 AM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
238

Very nice! Thanks for the example.

guraypp added a subscriber: guraypp.Aug 5 2020, 2:01 PM
kiranchandramohan accepted this revision.Aug 7 2020, 8:26 AM

LGTM. Please wait for @rriddle to approve.

mlir/test/Dialect/OpenACC/ops.mlir
188

Nit:

This revision is now accepted and ready to land.Aug 7 2020, 8:26 AM
clementval marked an inline comment as done.

New line at the end of file

clementval marked an inline comment as done.Aug 10 2020, 8:15 AM
rriddle accepted this revision.Aug 10 2020, 6:15 PM
rriddle added inline comments.
mlir/include/mlir/Dialect/OpenACC/OpenACCOps.td
23

nit: This looks like what you would put in the summary field.

39

This looks weird, remove?

80

Can you add in the Example: part? It is consistent with the other dialects.

87

nit: Move the verifier to later in the op definitions.

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
18

I think some of these can be trimmed.

29

I think this needs to be updated after a recent refactoring.

35

What unknown operations are being allowed here? Why do we need to do this?

42

nit: Drop llvm:: here, this along with many others are re-exported in the MLIR namespace.

82

nit: Just return this.

103

nit: This type doesn't look like it's being mutated, drop the reference specifier?

142

nit: Spell out auto here.

578

nit: Drop the trivial braces here.

clementval marked 12 inline comments as done.

Address review comments

@rriddle I pushed a new version of the patch with changes to address your comments. Are we good to go like this?

mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp
18

Yeah I have removed a bunch.

35

I'm gonna remove this since it is not necessary at this stage. Will add it again if there is a need later.