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

[abidmalikwaterloo]

Summary

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

[abidmalikwaterloo]

Made changes to alignment
Made AnyType to AnyInteger for map_type_modifier

[clementval]

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

151

Extra line doesn't seem to fit the file format.

466

Extra line doesn't seem to fit the file format.

565

Extra line doesn't seem to fit the file format.

674

What's the purpose of the region on enter data?

[abidmalikwaterloo]

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

674

If I am correct, it is defining the region within which the scope exit.

[abidmalikwaterloo]

Remove space as mentioned by the reviewer

[clementval]

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

674

I'm pretty sure enter data does not need an attached region. It's a standalone directive so no other operation will be nested inside its region. target exit data does not have one so these two operations are very close and it should be reflected in their MLIR design.

[abidmalikwaterloo]

made changes in the data enter operation

[clementval]

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

584

There still lines exceeding the 80 characters limit. Can you double check the whole patch so we can approve it.

[abidmalikwaterloo]

Correct indentation for comments

[clementval]

I have kind of lost track on this. Is this up to date and ready to go?

[abidmalikwaterloo]

Will try to finalize this week.

[abidmalikwaterloo]

Sorry for the delay. Will work on it

[shraiysh]

Looks okay to me, please wait for Kiran/Valentin's comments. Also, will you be adding assembly format/verifier for this later?

The summary says map clause is not supported but there are map operands in the patch. Can we get these two in sync - we should either update summary or remove map-related operands.

Also, please rebase before further reviews.

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

588

please put variables in documentation within backticks(`). This is because when the documentation is rendered on mlir.llvm.org, $ and _ interact to give unexpected text if backticks are not used.

[abidmalikwaterloo]

In D105255#3473574, @shraiysh wrote:

Looks okay to me, please wait for Kiran/Valentin's comments. Also, will you be adding an assembly format/verifier for this later?

Yes

The summary says the map clause is not supported but there are map operands in the patch. Can we get these two in sync - we should either update the summary or remove map-related operands.

Also, please rebase before further reviews.

Will do

[clementval]

In D105255#3474527, @abidmalikwaterloo wrote:

In D105255#3473574, @shraiysh wrote:

Looks okay to me, please wait for Kiran/Valentin's comments. Also, will you be adding an assembly format/verifier for this later?

Yes

The summary says the map clause is not supported but there are map operands in the patch. Can we get these two in sync - we should either update the summary or remove map-related operands.

Also, please rebase before further reviews.

Will do

Do you or did you work on the rest of the lowering to OpenMPIRBuilder/LLVM IR? Might be good to have the broader pictures sometime before designing the operation.

[abidmalikwaterloo]

In D105255#3474533, @clementval wrote:

In D105255#3474527, @abidmalikwaterloo wrote:

In D105255#3473574, @shraiysh wrote:

Looks okay to me, please wait for Kiran/Valentin's comments. Also, will you be adding an assembly format/verifier for this later?

Yes

The summary says the map clause is not supported but there are map operands in the patch. Can we get these two in sync - we should either update the summary or remove map-related operands.

Also, please rebase before further reviews.

Will do

Do you or did you work on the rest of the lowering to OpenMPIRBuilder/LLVM IR? Might be good to have the broader pictures sometime before designing the operation.

I put the line of action that we had in mind ( based on the OpenACC translation) in the FLANG meeting with other team members. I will work on the same line. My thinking is to work on translation and see what we need to change in the operation API at this level. But, I will clean and update the patch based on the feedback as it is hanging around too long.

[abidmalikwaterloo]

In D105255#3474527, @abidmalikwaterloo wrote:

In D105255#3473574, @shraiysh wrote:

Looks okay to me, please wait for Kiran/Valentin's comments. Also, will you be adding an assembly format/verifier for this later?

Yes

The summary says the map clause is not supported but there are map operands in the patch. Can we get these two in sync - we should either update the summary or remove map-related operands.

Also, please rebase before further reviews.

Will do

I have updated the summary.

[abidmalikwaterloo]

I have updated the summary.

[abidmalikwaterloo]

Do we have any comments on this? I can submit the "assembly format" later. Need the operation definition for the next step : lowering to LLVM IR

[shraiysh]

Please rebase, I cannot download this patch because it is too old :(

[abidmalikwaterloo]

Rebase the patch and clean some formatting as needed

[shraiysh]

Minor nits about documentation.

LGTM given that verifier and assembly format will be added soon for each of these as a separate patch soon. Please wait for atleast one more +1.

The OpenMP IRBuilder work does not have to wait for this patch to merge. You can start working on that while keeping this patch as a dependency.

clang/lib/Testing/CMakeLists.txt

6 ↗

(On Diff #429235)

nit: Merge conflict. Please remove.

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

588

nit: Ping on this comment. The variable names have not been put in backticks and this is going to cause issues with autogenerated documentation.

594

Data mapping is very similar to data privatization. Would it be a good idea to push this to the frontends while we have not concretely decided on openmp dialect privatization/mapping? If you have an idea on how to implement mapping in OpenMP Dialect to LLVM IR maybe we can go ahead with this.

CC @kiranchandramohan for suggestions regarding privatization.

661

It is not clear which element in this list corresponds to which map type. It would be nice if we could either document that here, or have separate operands for each map type. (same for other two constructs too)

[abidmalikwaterloo]

clang/lib/Testing/CMakeLists.txt

33 ↗

(On Diff #429235)

Are these okay? In D105584 these were not

[shraiysh]

clang/lib/Testing/CMakeLists.txt

33 ↗

(On Diff #429235)

No, this is not okay. There is some issue with these patches and I don't know what it is, but these unrelated changes should not be seen here. Thank you for pointing this out :)

[abidmalikwaterloo]

clang/lib/Testing/CMakeLists.txt

6 ↗

(On Diff #429235)

removed

33 ↗

(On Diff #429235)

removed them manually

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

594

@kiranchandramohan @raghavendhra I would like to keep it like this! Any comments

661

I feel keeping it in vector makes more sense. I will document it here. As per OpenMP Specifications:
The map-type can be to, from, tofrom, or alloc.

718

will document map type here as wekk

[abidmalikwaterloo]

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

661

Correction: map_type_modifier can be "always, close, and mapper"

[abidmalikwaterloo]

Revised the patcg according to the reviews of the reviewers.
clean the code and add some comments

[raghavendhra]

@abidmalikwaterloo ninja check-mlir is failing with your most upto date patch. Can you please double check?

Thanks!

[abidmalikwaterloo]

Update ops.mlir test cases. llvm-lit is working and all tests are passing

[clementval]

Why not adding the assemblyFormat directly for these operations like your patch D105584?

[abidmalikwaterloo]

In D105255#3643428, @clementval wrote:

Why not adding the assembly format directly for these operations like your patch D105584?

OK, I will do it!

[abidmalikwaterloo]

Added assembly format
changed the type of mape_type_modifier

[abidmalikwaterloo]

I just updated the patch. Something went wrong. I used arc diff --update <patch> for uploading. However, it seems it is not comparing the last submitted patch.

[raghavendhra]

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with usage of diff --update <patch>? Can you please check?

[abidmalikwaterloo]

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

@raghavendhra I tried to correct it but could not figure out the solution. Do you have any? I am not experienced with the phabricator framework. One solution is to submit another patch with all changes till today and abandon this one.

[raghavendhra]

In D105255#3707088, @abidmalikwaterloo wrote:

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

@raghavendhra I tried to correct it but could not figure out the solution. Do you have any? I am not experienced with the phabricator framework. One solution is to submit another patch with all changes till today and abandon this one.

Sorry, I am new to phabricator as well. I use "arc diff" to upload my revisions for review after addressing review comments. If no one responds probably I am fine with new review and abandoning this one.

[clementval]

In D105255#3707495, @raghavendhra wrote:

In D105255#3707088, @abidmalikwaterloo wrote:

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

@raghavendhra I tried to correct it but could not figure out the solution. Do you have any? I am not experienced with the phabricator framework. One solution is to submit another patch with all changes till today and abandon this one.

Sorry, I am new to phabricator as well. I use "arc diff" to upload my revisions for review after addressing review comments. If no one responds probably I am fine with new review and abandoning this one.

Did you try arc diff --update D105255 from your branch?

[abidmalikwaterloo]

In D105255#3707533, @clementval wrote:

In D105255#3707495, @raghavendhra wrote:

In D105255#3707088, @abidmalikwaterloo wrote:

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

@raghavendhra I tried to correct it but could not figure out the solution. Do you have any? I am not experienced with the phabricator framework. One solution is to submit another patch with all changes till today and abandon this one.

Sorry, I am new to phabricator as well. I use "arc diff" to upload my revisions for review after addressing review comments. If no one responds probably I am fine with new review and abandoning this one.

Did you try arc diff --update D105255 from your branch?

yes, I tried this. Actually, I updated the patch. through this

[clementval]

In D105255#3708258, @abidmalikwaterloo wrote:

In D105255#3707533, @clementval wrote:

In D105255#3707495, @raghavendhra wrote:

In D105255#3707088, @abidmalikwaterloo wrote:

In D105255#3706602, @raghavendhra wrote:

@abidmalikwaterloo Can you please check your last patch pushed for review? https://buildkite.com/llvm-project/diff-checks/builds/117796 states it can not apply your patch something to do with the usage of diff --update <patch>? Can you please check?

@raghavendhra I tried to correct it but could not figure out the solution. Do you have any? I am not experienced with the phabricator framework. One solution is to submit another patch with all changes till today and abandon this one.

Sorry, I am new to phabricator as well. I use "arc diff" to upload my revisions for review after addressing review comments. If no one responds probably I am fine with new review and abandoning this one.

Did you try arc diff --update D105255 from your branch?

yes, I tried this. Actually, I updated the patch. through this

The problem is likely that the commit before the commit of this patch is not in tree,

The easiest way is to create a new branch from main and cherry-pick the commit you need for this patch on the top of it. Then you just use arc diff --update and it should work.

[abidmalikwaterloo]

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

[clementval]

In D105255#3723865, @abidmalikwaterloo wrote:

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

Looks like it's not the complete revision. There are almost no diffs anymore.

[abidmalikwaterloo]

In D105255#3723940, @clementval wrote:

In D105255#3723865, @abidmalikwaterloo wrote:

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

Looks like it's not the complete revision. There are almost no diffs anymore.

My OS system crashed last week. I am working on it.