Page MenuHomePhabricator

[mlir]Add Allocate Clause to OMP Parallel Operation Definition
ClosedPublic

Authored by Rin on Sep 15 2020, 4:15 AM.

Diff Detail

Event Timeline

Rin created this revision.Sep 15 2020, 4:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 4:15 AM
Rin requested review of this revision.Sep 15 2020, 4:15 AM
ftynse added a subscriber: ftynse.Sep 15 2020, 4:42 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
68–70

Nit: "variable" is unclear in MLIR context, we only have values.

Also please terminate sentences with a dot, this is required for documentation, comments, etc. except error messages.

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

Nit: please use backticks consistently (here and above)

137

Nit: prefer early-returns, i.e. if (varAllocate.empty()) return; <current-code-with-less-indentation>

138–140

Nit: this does not look properly formatted

141–145

mlir::interleaveComma will make it easier to read

200–205

SmallVector is re-exported into the mlir namespace, please drop llvm:: here and above.

343

Nit: drop trivial braces

DavidTruby added inline comments.Sep 15 2020, 8:00 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
88

Is it possible for us to "fix" the type of allocate, since we know it'll always be an intptr_t? Genuine question, I'm not sure if this is possible/desirable in mlir.

138–140

I also checked this because it looked odd, but this is how clang-format leaves this for me as well.

200–205

Shall we remove the llvm:: prefix on the existing variables as a separate patch, so as not to expand the scope of this one? I can submit that patch since those SmallVectors were added by me in the first place.

mlir/test/Dialect/OpenMP/ops.mlir
103

Question in general for these clauses, is it possible to elide any of these types or do they need to be specified each time?

This applies to the other parallel clauses as well as just this one: when I was writing those I couldn't find a way to elide the types.

Generally I think it doesn't make those ones that much more verbose, but here it does make them quite verbose due to the necessary "->" syntax etc. We might find this verbosity gets even worse for future clauses on other omp directives.

ftynse added inline comments.Sep 15 2020, 8:07 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
88

MLIR's equivalent of intptr_t is index, but I suppose this also needs to be compatible with LLVM dialect integers.

138–140

Would it re-split lines if this had been p << " " << "allocate" << "("; ? It also looks like p << " allocate("; is just fine here.

200–205

Yes please!

mlir/test/Dialect/OpenMP/ops.mlir
103

It's possible to omit types in the pretty syntax if the parser can infer them within the scope of one operation. E.g., load/store expect subscript types to be index so they can just call parser.getBuilder().getIndexType() instead of requiring it to be present in the assembly form.

DavidTruby added inline comments.Sep 15 2020, 8:10 AM
mlir/test/Dialect/OpenMP/ops.mlir
103

So, it is possible to omit them in e.g. the if clause that I added before (since we know that the condition is always an i1) but not possible for example in copyin where the type is AnyType? Even though that type is set when that variable is passed into the function itself?

ftynse added inline comments.Sep 15 2020, 8:14 AM
mlir/test/Dialect/OpenMP/ops.mlir
103

Ultimately you need to be able to construct an operation given only the information present in the assembly syntax. At parsing time, the operand values may not have been created yet, so you cannot query their type.

Rin added inline comments.Sep 15 2020, 8:23 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
138–140

I'll try this and see if it formats it weird again.

Rin updated this revision to Diff 292133.Sep 16 2020, 1:15 AM

Address review comments

Rin added a reviewer: ftynse.Sep 16 2020, 1:15 AM
Rin retitled this revision from Add Allocate Clause to MLIR Parallel Operation Definition to [flang]Add Allocate Clause to MLIR Parallel Operation Definition.
Rin marked 4 inline comments as done.
Rin retitled this revision from [flang]Add Allocate Clause to MLIR Parallel Operation Definition to [mlir]Add Allocate Clause to MLIR Parallel Operation Definition.Sep 16 2020, 6:48 AM
ftynse retitled this revision from [mlir]Add Allocate Clause to MLIR Parallel Operation Definition to [mlir]Add Allocate Clause to OMP Parallel Operation Definition.Sep 17 2020, 7:10 AM
Rin updated this revision to Diff 292748.Sep 18 2020, 3:26 AM

Fix Failing Test

There are review comments that remain unaddressed on this. Please address or comment as to why you chose not to address them.

Rin marked an inline comment as done.Sep 21 2020, 7:46 AM
Rin added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
141–145

I've tried to use that and it seemed to make the code more complex and so l decided not to use it.

343

When adding the allocate and allocator parameters I followed the structure of the already implemented clauses, which use braces, so I thought it would be a good idea to keep those the same.

Rin updated this revision to Diff 293174.Sep 21 2020, 7:50 AM

Address review comments.

Rin marked an inline comment as done.Sep 21 2020, 7:50 AM
Rin marked an inline comment as done.
kiranchandramohan requested changes to this revision.Sep 21 2020, 2:17 PM

Comments inline.

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

There is a mismatch in the order of arguments between the caller and the callee here.
In the caller the first argument is allocate_vars and the second is allocator_vars. Here in the callee the order is reversed.

343

I believe this suggestion is as per llvm coding standards.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Agree that there is other code which also needs to be changed. Perhaps in a different patch.

This revision now requires changes to proceed.Sep 21 2020, 2:17 PM
Rin updated this revision to Diff 293390.Sep 22 2020, 2:50 AM

Address review comments

Rin marked 3 inline comments as done.Sep 22 2020, 2:50 AM
DavidTruby added inline comments.Sep 22 2020, 6:47 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
200–205

This is done for the existing ones in D88025

Rin updated this revision to Diff 293744.Sep 23 2020, 8:15 AM

Add verifier

LGTM subject to addressing one comment inline. Would it be possible to add an error check for allocate parsing?
Please wait for approval from other reviewers.

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

Nit question: Should this be renamed to verifyParallelOp?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
70–73

Unless I am missing something, this needs correction. Suggestion below, please consult other examples and suitably modify before use.

/// operand-and-type-list ::= `(` allocate-operand-list `)`
/// allocate-operand-list :: = allocate-operand | 
                                               allocate-operand `,` allocate-operand-list
/// allocate-operand :: = ssa-id-and-type -> ssa-id-and-type
/// ssa-id-and-type ::= ssa-id `:` type
This revision is now accepted and ready to land.Mon, Sep 28, 8:43 AM
Rin updated this revision to Diff 294730.Mon, Sep 28, 9:28 AM

Address review comments

Rin marked 2 inline comments as done.Mon, Sep 28, 9:29 AM
SouraVX accepted this revision.EditedTue, Sep 29, 4:44 AM
SouraVX added a subscriber: SouraVX.

Since it's already approved, feel free to remove test case modification and commit no need for another revision :)
EDIT: if other reviewers comments are already addressed.

flang/unittests/Lower/OpenMPLoweringTest.cpp
86 ↗(On Diff #294730)
Rin added a comment.Tue, Sep 29, 7:15 AM

@ftynse Does the patch look alright?

Rin updated this revision to Diff 297263.Fri, Oct 9, 9:37 AM

Add aditional check and remove unnecessary test

Rin added inline comments.Fri, Oct 9, 9:40 AM
flang/lib/Lower/OpenMP.cpp
112

Here these are just a placeholder for the allocate and allocator. They will be replaced with the actual lowering implementation for the allocate clause in my next patch.

Rin added a reviewer: rriddle.Fri, Oct 9, 9:41 AM
This revision was automatically updated to reflect the committed changes.
SouraVX added inline comments.Thu, Oct 15, 8:22 AM
flang/lib/Lower/OpenMP.cpp
112

This may cause conflict in FIR-dev. Could you please take a look ?