Page MenuHomePhabricator

[MLIR] Support for flush operation, and translating the same to LLVM IR
ClosedPublic

Authored by kiranktp on May 14 2020, 4:54 AM.

Details

Summary

This patch adds support for flush operation in OpenMP dialect and translation of this construct to LLVM IR.
The OpenMP IRBuilder is used for this translation.
The patch includes code changes and testcase modifications.

Diff Detail

Event Timeline

kiranktp created this revision.May 14 2020, 4:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2020, 4:54 AM
ftynse requested changes to this revision.May 14 2020, 5:26 AM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
320

Let's not use Fortran syntax in C++ code

321

This comment doesn't seem to belong here

mlir/test/Dialect/OpenMP/ops.mlir
27

This doesn't actually check that you print back operands

28

Could we rather use the custom format?

This revision now requires changes to proceed.May 14 2020, 5:26 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
322

Is this a TODO?
Do we require another version of CreateFlush in the OpenMP Builder to support the list?

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

Shall we have more functions to test? Maybe one for those with arguments and one for those without? Also, rename this function.

20

Is it possible to check that the arguments are also passed correctly for this version of omp.flush?

kiranktp added inline comments.May 14 2020, 10:41 AM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
320

I will remove the Fortran syntax

321

This function is specific to openmp Op convertion and also this comment is very generic to flush construct (both C/Fortran). So I placed it here.

322

Openmp runtime will discard the list even if its passed. This is the same behavior with clang and legacy flang as well. The list will be discarded for flush construct.
Same has been documented in the OpenMP standard as well.
From OpenMP 4.5 Standard Doc:
"An implementation may implement a flush with a list by ignoring the list, and treating it the same as a flush without a list."
"Note – Use of a flush construct with a list is extremely error prone and users are strongly discouraged from attempting it."
I think this is the reason, there is only one version of CreateFlush without this list.
If the decision is to process the list and pass it on to IR builder, another version of CreateFlush has to be implemented.
In spite of this, the list will be discarded and a call to __kmpc_flush will be inserted without the list in IR builder.

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

I will add a separate function with arguments and give a meaning full name.

kiranktp updated this revision to Diff 264537.May 17 2020, 11:01 PM

Hi @ftynse, @kiranchandramohan,
I have incorporated the review comments.

kiranktp marked 15 inline comments as done.May 17 2020, 11:05 PM
kiranktp added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
321

I have added the comment around flush construct handling.

mlir/test/Dialect/OpenMP/ops.mlir
27

I have updated the test case to validate the operands as well.

28

The current changes looks fine?

mlir/test/Target/openmp-llvm.mlir
20

Anyway the argument list is being discarded, we need not check for correctness of the arguments.

kiranktp marked 4 inline comments as done.May 17 2020, 11:06 PM
ftynse accepted this revision.May 18 2020, 5:06 AM
ftynse added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
339

Please comply with the linter (run git clang-format on your patch)

This revision is now accepted and ready to land.May 18 2020, 5:06 AM
kiranktp updated this revision to Diff 264848.May 19 2020, 4:07 AM

Fixed formatting issue.

kiranktp marked an inline comment as done.May 19 2020, 4:08 AM
This revision was automatically updated to reflect the committed changes.