Page MenuHomePhabricator

[MLIR] Support for taskwait and taskyield operations, and translating the same to LLVM IR
ClosedPublic

Authored by kiranktp on Apr 7 2020, 2:05 AM.

Details

Summary

This patch adds support for taskwait and taskyield operations in OpenMP dialect and translation of the these constructs to LLVM IR. The OpenMP IRBuilder is used for this translation.
The patch includes code changes and a testcase modifications.

Diff Detail

Event Timeline

kiranktp created this revision.Apr 7 2020, 2:05 AM
mehdi_amini added inline comments.Apr 7 2020, 2:52 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
75

The declarative assembly syntax may be shorter here :)

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
331

The sequence may be representable with a TypeSwitch here?

Thanks @kiranktp for working on this. Looks OK to me.

Just pinging @rogfer01 since he wrote the portion of the builder for taskwait and taskyield.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
310–311

There might be more than one LLVM IR operation corresponding to an OpenMP MLIR operation.

Would the following be better?
Given an OpenMP MLIR operation, create the corresponding LLVM IR (including OpenMP runtime calls).

mlir/test/Target/openmp-llvm.mlir
4–7

Should we move all these checks closer to the operation? What is the MLIR/LLVM style for this?

SouraVX added inline comments.Apr 7 2020, 10:47 PM
mlir/test/Target/openmp-llvm.mlir
4–7

They can be moved closer to operations https://llvm.org/docs/CommandGuide/FileCheck.html, it's also better for better readibility and cleanly extending test case further in future.

kiranktp marked 4 inline comments as done.Apr 8 2020, 2:45 AM
kiranktp added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
75

Did you mean, this declarative syntax is too short here. Could you please elaborate a bit on this?

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
310–311

But this function will be called for single OpenMP MLIR operation.
Nevertheless, your wording for comment is neutral and more clear. I will incorporate them.

331

I was not aware of TypeSwitch feature. Thanks for the suggestion Mehdi. I will modify as you suggested.

mlir/test/Target/openmp-llvm.mlir
4–7

Thanks for the review Kiran.
Yes, we can move these checks closure to the operation. I will do this modification.

kiranktp updated this revision to Diff 255937.Apr 8 2020, 2:48 AM
kiranktp marked 4 inline comments as done and an inline comment as not done.
kiranktp updated this revision to Diff 255960.Apr 8 2020, 4:34 AM
mehdi_amini added inline comments.Apr 8 2020, 8:08 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
75

Sorry I was terse, I meant to use a different mechanism here possibly, look the section about « Declarative Assembly Format» here : https://mlir.llvm.org/docs/OpDefinitions/

kiranktp updated this revision to Diff 256249.Apr 9 2020, 4:57 AM

Incorporated all review comments.

kiranktp marked an inline comment as done.Apr 9 2020, 4:57 AM
mehdi_amini accepted this revision.Apr 9 2020, 1:15 PM
This revision is now accepted and ready to land.Apr 9 2020, 1:15 PM

Hi Mehdi, Thanks for the review. I do not have commit access. Could you please merge this code on my behalf.?

This revision was automatically updated to reflect the committed changes.