This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP]Add custom parser and pretty printer for target construct
Needs RevisionPublic

Authored by abidmalikwaterloo on Jun 4 2021, 2:59 PM.

Details

Summary

added custom parser and pretty printer for target construct with if, thread_limit, nowait, and device clauses

Diff Detail

Event Timeline

abidmalikwaterloo requested review of this revision.Jun 4 2021, 2:59 PM
abidmalikwaterloo retitled this revision from Add custom parser and pretty printer for target construct to [MLIR][OpenMP}Add custom parser and pretty printer for target construct.Jun 4 2021, 5:02 PM
abidmalikwaterloo retitled this revision from [MLIR][OpenMP}Add custom parser and pretty printer for target construct to [MLIR][OpenMP]Add custom parser and pretty printer for target construct.Jun 4 2021, 5:02 PM
kiranchandramohan requested changes to this revision.Jun 7 2021, 10:54 AM

Requesting a few changes.

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

Nit: Is there an extra space before '=' now?

260–301

You can remove this and add D102816 as a parent patch.

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

Is this required?

790–801

This build function is probably not required. The autogenerated one will probably do this I guess. Do you need this for a particular reason?

884

Nit: Please add an empty line after this function.

mlir/test/Dialect/OpenMP/ops.mlir
93

Nit: Accidental change?

166–167

Nit: some formatting accidentally added?

284–297

This test is using the generic syntax and probably not required.

304

Nit: One or two spaces at the beginning here.

308

Add the if, thread_limit, nowait tests here or in another omp.target op.

309

Nit: One or two spaces at the beginning here.

This revision now requires changes to proceed.Jun 7 2021, 10:54 AM
clementval requested changes to this revision.Jun 7 2021, 12:16 PM
clementval added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
790–801

+1

809
810

nit: added spaces just to fit with previous style

831

Might be good to have these keywords declared as constexpr somewhere (extraClassDeclaration) so you can reuse them in the printer.

857

Would be nice to have the attribute name declared in the class and not hard-coded here. It's pretty sure that it will be used in conversion and translation.

abidmalikwaterloo marked 16 inline comments as done.Jun 22 2021, 3:51 PM

Took care of the comments.

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

No. I will correct it.

260–301

I have made D102816 a parent task. However, I am introducing

let parser = [{ return parseTargetOp(parser, result); }];
let printer = [{ return printTargetOp(p, *this); }];

here.

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

No, I will remove it.

810

OK.

831

OK. Should we do it to the other operations as well? ParallelOps

857

OK

884

OK

mlir/test/Dialect/OpenMP/ops.mlir
93

Yes.. Will fix it

166–167

Got it.

abidmalikwaterloo marked 9 inline comments as done.

Took care of the comments from the reviewers.

clementval added inline comments.Jul 12 2021, 1:14 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
295

Not aligned correctly.

300

Indentation is not correct for this line and the rest inside should be indented as well.

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

Why not using SmallVector?

abidmalikwaterloo marked an inline comment as done.Jul 12 2021, 5:22 PM
abidmalikwaterloo added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
808

I will change it to SmallVector<int,3>

abidmalikwaterloo marked 3 inline comments as done.Jul 12 2021, 6:02 PM

Made changes according to the reviewrs' comments and feedback.

Remove std:array<int,3> and replace it with SmallVector<int,3>

clementval requested changes to this revision.Jul 20 2021, 8:48 AM
clementval added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
26

What's the reason to remove this blank line?

171

Unrelated change?

808

It's recommended to not specify the number of inlined elements for SmallVector unless you have a good reason. I would just use SmallVector<unsigned> in this case.

813

Could be constexpr?

This revision now requires changes to proceed.Jul 20 2021, 8:48 AM
abidmalikwaterloo marked 2 inline comments as done.Jul 22 2021, 9:28 AM
abidmalikwaterloo added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
26

My mistake. Will put it back.

171

I will put the space. Accidentally removed.

808

SmallVector<int> should be fine.

813

OK

abidmalikwaterloo marked 2 inline comments as done.

Add spaces which were accidently removed OpenMPDialect.cpp
Change SmallVector<int,3> to SmallVector<int>
Change const int to constexpr int
Made changes as per reviewer's comments

kiranchandramohan requested changes to this revision.Aug 10 2021, 3:21 PM

Looks mostly OK. A few more comments.

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

Nit: Spelling : eliminates

293

I think this builder is doing the same thing that the default builder does. So is it required?

Also, the if_expr, device and thread_limit are operands and not attributes. So I think their type should be Value and not IntegerAttr.

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

Nit: ssa-id -> ssa-id-and-type ?

865–866

I guess @clementval's SmallVector comment about not specifying the size applies here as well.

This revision now requires changes to proceed.Aug 10 2021, 3:21 PM
abidmalikwaterloo marked 2 inline comments as done.Sep 2 2021, 1:24 PM
abidmalikwaterloo added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
795

Got it.

865–866

Ok

abidmalikwaterloo marked an inline comment as done.

Did the code alignments and corrected the codes as per reviewers' comments.

There are still some comments not addressed so far. I guess once you have addressed all the comments we can move forward,

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

Seems like unrelated change.

abidmalikwaterloo marked 2 inline comments as done.Oct 5 2021, 10:27 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
293

No expert in "TableGen". We should remove it if this is the case.

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

I did it in line with the comments at line# 866. Though forgot to correct that.

clementval added inline comments.Oct 5 2021, 11:52 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
293

Yeah I think you can remove it. Seems like you don't need a specific builder for the time being.

abidmalikwaterloo marked 3 inline comments as done.Oct 18 2021, 2:04 PM

remove the builder as per suggested by the reviewer

Hi @abidmalikwaterloo Thanks for this patch. There are a couple of issues with the styling which is causing the builds to fail. For example trailing spaces, 80-character line wraps and maybe some indentation. I have a few comments.

In the OpenMP 5.0 standard, there is no thread_limit clause for the target construct. Is it necessary?

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

These functions are used only in the parser, and they can be removed - the strings can directly be used. (You will not have to use them too once you use the parseClauses function).

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

This code can now be condensed using the parseClauses function in the latest version of the file (like SectionsOp and ParallelOp).

Is there still any development on this?

Will try to finalize this week.

peixin added a subscriber: peixin.Jan 18 2022, 3:40 AM

In the OpenMP 5.0 standard, there is no thread_limit clause for the target construct. Is it necessary?

@shraiysh I think this patch is following OpenMP 5.1 standard. In line 261 of OpenMPOps.td, section 2.14.5 is refered and it is the section in openmp 5.1 standard.

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

Should there be one space between "target", and [AttrSizedOperandSegments]?

287

The device modifier is missed. Will it be implemented later?

Are there any other issues with the patch? I will clean it and submit it?

Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2022, 9:40 AM
kiranchandramohan requested changes to this revision.Apr 25 2022, 10:20 AM

Unfortunately, there have been several developments since we last looked at this patch. This includes using assembly format for the printer/parser instead of the custom printTargetOp/parseTargetOp. See the following for reference.
https://github.com/llvm/llvm-project/blob/6f73bd781305266a747055875ce8352e5a36c809/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L115

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

You can use I1 here instead of AnyInteger.

This revision now requires changes to proceed.Apr 25 2022, 10:20 AM

Unfortunately, there have been several developments since we last looked at this patch. This includes using assembly format for the printer/parser instead of the custom printTargetOp/parseTargetOp. See the following for reference.
https://github.com/llvm/llvm-project/blob/6f73bd781305266a747055875ce8352e5a36c809/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L115

Does this mean we need to resubmit a new patch for TargetOp?

This has been taken care of. I think need to abandon this patch?

This has been taken care of. I think need to abandon this patch?

You can update this patch itself here.

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

Should we need to do it? I can come to it later for another

This patch need rebasing and some formating.

shraiysh added inline comments.May 13 2022, 12:47 PM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
817

Not anymore. We should use the assemblyformat now and eliminate custom parsers entirely.

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

I see the assembly format for the target operation. Does this mean the patch is redundant?

clementval resigned from this revision.Apr 6 2023, 11:19 AM