Page MenuHomePhabricator

[mlir][openmp] Custom syntax for `omp.target` operation
ClosedPublic

Authored by alexbatashev on Jan 17 2022, 10:57 PM.

Details

Summary

Add a custom parser and printer for omp.target operation.

Diff Detail

Event Timeline

alexbatashev created this revision.Jan 17 2022, 10:57 PM
alexbatashev requested review of this revision.Jan 17 2022, 10:57 PM
alexbatashev retitled this revision from [mlir][openmp] Improve OMP target clause support to [mlir][openmp] Improve `omp.target` operation support.Jan 17 2022, 11:12 PM

This is useful support. Looks good - a comment below.

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

Use AttrSizedOperandSegments::getOperandSegmentSizeAttr().

bondhugula accepted this revision.Jan 18 2022, 2:48 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
406

Update doc above: I think there isn't a need to define these other than mentioning that they have the same meaning as in the OpenMP standard?

This revision is now accepted and ready to land.Jan 18 2022, 2:48 AM

address reviewers feedback

alexbatashev marked 2 inline comments as done.Jan 18 2022, 3:12 AM

Thanks @bondhugula! I've switched to AttrSizedOperandSegments::getOperandSegmentSizeAttr() instead of referencing the attribute by name and updated docs to reflect that new arguments have the same meaning as in OpenMP standard.

bondhugula accepted this revision.Jan 18 2022, 2:46 PM

@bondhugula can you please submit this patch on my behalf? I don't have merge rights to LLVM. Please, use these credentials: Alexander Batashev <alexander.batashev@intel.com>

Some changes were missing from this differential. Return them back.

FWIW, I couldn't see changes from my first changeset in this differential, so I squashed both commits into one and uploaded them again.

It'll be good if someone closely working on the omp dialect also reviews this. I've added a few of them.

Thanks for this patch. In general, we are a bit reluctant to add the privatisation clauses to more operations since the discussion for handling privatisation (https://llvm.discourse.group/t/rfc-privatisation-in-openmp-dialect/3526) in the MLIR dialect has not concluded. Flang which uses the OpenMP dialect deals with privatisation when it lowers to MLIR. We should be back to discussing privatisation soon.

We can take the other changes.

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

Nit: num_threads -> thread_limit.

This revision now requires review to proceed.Jan 19 2022, 3:50 AM

Removed private and firstprivate clauses

alexbatashev retitled this revision from [mlir][openmp] Improve `omp.target` operation support to [mlir][openmp] Custom syntax for `omp.target` operation.Jan 20 2022, 12:57 AM
alexbatashev edited the summary of this revision. (Show Details)

@kiranchandramohan thanks for letting me know and pointing to that interesting discussion!

This revision is now accepted and ready to land.Jan 25 2022, 9:11 AM

@kiranchandramohan can you please submit this patch on my behalf? I don't have merge rights to LLVM. Please, use these credentials: Alexander Batashev <alexander.batashev@intel.com>

I have submitted the patch for you @alexbatashev. It seems that you have a few patches submitted to llvm, I think you can ask for commit access so that you can merge patches yourself.
You can find the details for obtaining commit access from the following link.
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access