Page MenuHomePhabricator

[MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.
Needs RevisionPublic

Authored by abidmalikwaterloo on Aug 15 2022, 12:39 PM.

Details

Summary

This includes a basic implementation for the OpenMP target data, exit data, and enter data constructs
operation.

This patch will replace the https://reviews.llvm.org/D105255/new/ (D105255)
It is the latest update of D105255
One can see the comments in the previous patch.
Every issue/comment has been taken care of from D105255

Diff Detail

Event Timeline

abidmalikwaterloo requested review of this revision.Aug 15 2022, 12:39 PM
abidmalikwaterloo edited the summary of this revision. (Show Details)
abidmalikwaterloo added a reviewer: clementval.
This revision is now accepted and ready to land.Aug 17 2022, 10:37 PM

In D105255 we discussed about adding assembly format directly. I don't see it here.

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

Any reason to add comment in the description that will be comment anyway? Same below.

abidmalikwaterloo marked an inline comment as done.Aug 18 2022, 6:34 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
832

I was asked by a reviewer in D105255 to clarify the values of the modifier through comments.

clementval added inline comments.Aug 18 2022, 7:24 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
832

What I meant is that the description block is already comments so you don't need to add ///.

abidmalikwaterloo marked 2 inline comments as done.Aug 18 2022, 8:11 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
832

Ah! :) Got it. I will remove it.

raghavendhra added inline comments.Aug 18 2022, 4:58 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
865

Are you planning to add verifier in future patches?

940

Same as above, Are you planning to add verifier in future patches?

1015

Same as above, Are you planning to add verifier in future patches?

abidmalikwaterloo marked 4 inline comments as done.Aug 19 2022, 12:40 PM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
865

Yes

940

yes

1015

yes

clementval added inline comments.Aug 19 2022, 1:31 PM
mlir/test/Dialect/OpenMP/ops.mlir
389

Can you switch to the custom format form?

394

Same here

397

Same here.

399–400

Not needed for this test.

TIFitis added inline comments.Sun, Aug 28, 5:29 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
915–916

Do we need both $nowait_operand and $nowait?

990–991

Same here. Do we need both $nowait_operand and $nowait?

abidmalikwaterloo marked 5 inline comments as done.Mon, Sep 12, 6:22 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
915–916

No. Thanks for pointing this out. I overlooked it. It will be corrected.

TIFitis requested changes to this revision.Thu, Sep 15, 11:02 AM
TIFitis added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

When multiple map clauses are supplied, it is not possible to associate the map_type_modifier to its respective map.

Eg:

!$omp target enter data map(to: a) map(always alloc: b) can be lowered to something like
omp.target_enter_data map_to(%0 : !fir.ref<i32>) map_alloc(%1 : !fir.ref<i32>) map_type_modifier(%c4_i32 : i32)

It is not possible to deduce if the map_type_modifier %c4_i32 is for a or b.

I'm not sure if something like Variadic<Optional<AnyInteger>, AnyType> is supported then we can change all the map_operands' type to that. If not we might need a map_type_modifier for each map_operand type.

This revision now requires changes to proceed.Thu, Sep 15, 11:02 AM
TIFitis added inline comments.Thu, Sep 15, 11:16 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
917–923

Only map_to and map_alloc are allowed types for EnterData. We can perhaps get rid of the remaining map_operand types.

992–998

Only map_from, map_release and map_delete are allowed types for ExitData. We can perhaps get rid of the remaining map_operand types.

abidmalikwaterloo marked an inline comment as done.Thu, Sep 15, 11:30 AM
abidmalikwaterloo added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

One possibility is to make it a vector for each map_type_modifier. The length is equal to map clauses. The index number can be used to associate the corresponding map clause in the construct and '1' will tell whether the map_type_modifier is active for the corresponding clause. But, `Variadic<Optional<AnyInteger>, AnyType> option seems more appropriate if available.

TIFitis added inline comments.Thu, Sep 15, 11:44 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

I thought of using the index of map_type_modifier but that still doesn't solve the issue.

Eg,
omp target enter data map(to: a) map(always alloc: b) and
omp target enter data map(alloc: b) map(always to: a)

both get lowered to the same mlir

omp.target_enter_data map_to(a) map_alloc(b) map_type_modifier(false, true).

This is because upon lowering we lose information on ordering of map clauses from source.

clementval added inline comments.Thu, Sep 15, 12:03 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

The value in the map_type_modifer should be a combination of the different map type modifiers for the operand at the same position and not just true false. The runtime and OpenMP IR Builder are using this already.

TIFitis added inline comments.Thu, Sep 15, 1:38 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
846

Thanks for clearing that, I was wondering the same.

I think the current implementation is workable, but something like Variadic<Optional<AnyInteger>, AnyType> may be more preferrable.