Page MenuHomePhabricator

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

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.Mon, Jul 12, 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.Mon, Jul 12, 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.Mon, Jul 12, 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.Tue, Jul 20, 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.Tue, Jul 20, 8:48 AM
abidmalikwaterloo marked 2 inline comments as done.Thu, Jul 22, 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