Page MenuHomePhabricator

[MLIR][OpenMP]Basic OpenMP target operation
ClosedPublic

Authored by abidmalikwaterloo on May 19 2021, 4:29 PM.

Details

Summary

This includes a basic implementation for the OpenMP target
operation. Currently, the if, thread_limit, private, shared, device, and nowait clauses are included in this implementation.

Co-authored-by: Kiran Chandramohan <kiran.chandramohan@arm.com>

Diff Detail

Event Timeline

abidmalikwaterloo requested review of this revision.May 19 2021, 4:29 PM
abidmalikwaterloo retitled this revision from This includes a basic implementation for OpenMP target operation. The if, device, thread_limit and nowait. to Basic OpenMP target operation.May 19 2021, 4:34 PM
abidmalikwaterloo edited the summary of this revision. (Show Details)
ftynse accepted this revision.May 20 2021, 1:34 AM
ftynse added a subscriber: ftynse.

LGTM with comments addressed.

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

Nit: spurious empty line

291–292

Let's drop these empty lines please.

mlir/test/Dialect/OpenMP/ops.mlir
285

Please use full sentences with the first letter capitalized and a full stop in comments (https://llvm.org/docs/CodingStandards.html#commenting).

286

We usually align these with the line below.

This revision is now accepted and ready to land.May 20 2021, 1:34 AM
abidmalikwaterloo marked 4 inline comments as done.May 21 2021, 7:50 AM

Took care of the comments.

kiranchandramohan requested changes to this revision.May 21 2021, 8:13 AM

I am requesting a change for the nowait attribute. Have a few questions as well.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
285–288

Can nowait be a unit attribute like in the worksharing loop (UnitAttr:$nowait)?

Does AnyInteger cover boolean types as well?

Will AnyInteger work with out-of-tree dialects like FIR? I guess it will because FIR now uses mlir integer types.

Can the suffix _op be dropped?

mlir/test/Dialect/OpenMP/ops.mlir
287

Nit: space before %device.

This revision now requires changes to proceed.May 21 2021, 8:13 AM
abidmalikwaterloo marked 2 inline comments as done.May 24 2021, 6:04 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
285–288

I went through the OpenACC dialect, work-sharing loop op, and MLIR operation definition specifications. I agree, making nowait a unit attribute makes sense.

abidmalikwaterloo marked an inline comment as done.

Took care of the feedback and comments.

Thanks @abidmalikwaterloo for the patch!

Couple of NITs:

  • Please prefix the patch title with [mlir][OpenMP], this is convention we/community follow :)
  • And if possible can you make the title/subject/summary more specific to the intent, usually people first read those to get an idea :)
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
285–288

In the OpenMP Lowering flow, the OpenMP operation can exist along with the LLVM dialect so the Integer that an OpenMP clause can take can be an LLVM Integer. I believe the LLVM Integer was made similar to the mlir Integer but I can see the existence of LLVM_AnyInteger.
@ftynse Is there any difference between LLVM_AnyInteger and AnyInteger. They use the same predicate. If there is no difference can LLVM_AnyInteger be removed?

// Type constraint accepting LLVM integer types.
def LLVM_AnyInteger : Type<CPred<"$_self.isa<::mlir::IntegerType>()">, "LLVM integer type">;

// Any integer type irrespective of its width and signedness semantics.
def AnyInteger : Type<CPred<"$_self.isa<::mlir::IntegerType>()">, "integer", "::mlir::IntegerType">;
abidmalikwaterloo retitled this revision from Basic OpenMP target operation to [MLIR][OpenMP}Basic OpenMP target operation.May 25 2021, 2:55 AM
abidmalikwaterloo retitled this revision from [MLIR][OpenMP}Basic OpenMP target operation to [MLIR][OpenMP]Basic OpenMP target operation.
abidmalikwaterloo added a project: Restricted Project.
abidmalikwaterloo edited the summary of this revision. (Show Details)May 25 2021, 3:07 AM
clementval added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
285

Shouldn't if_expr be of type I1?

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

@clementval being boolean, it can be. I tried to keep the types close to the parallel operation

285–288

@ftynse any feedback on Kiran's comments.

abidmalikwaterloo marked an inline comment as done.Jun 3 2021, 4:29 PM

LGTM. One request for change.

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

Please switch to i1, we can generalise later if required.

This revision is now accepted and ready to land.Jun 6 2021, 11:40 PM
ftynse added inline comments.Jun 7 2021, 1:26 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
285–288

LLVM dialect no longer has its own integer type and uses built-in integers. A cleanup to remove LLVM_AnyInteger predicate is welcome.

abidmalikwaterloo marked an inline comment as done.Jun 9 2021, 2:39 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
285

Ok will do

abidmalikwaterloo marked 2 inline comments as done.Jun 21 2021, 4:08 PM

Made changes according to the comments from the reviewers.
SI32 for If condition changes to I1 (boolean).

@abidmalikwaterloo Do you have commit access? If not, I can submit this for you.

@kiranchandramohan I am not sure if I have commit access. You can do this for me.

This revision was automatically updated to reflect the committed changes.