Page MenuHomePhabricator

[MLIR][OpenMP] Conversion pattern to convert OpenMP ops to OpenMP ops with LLVM dialect
ClosedPublic

Authored by kiranchandramohan on Aug 20 2020, 2:46 AM.

Details

Summary

Adding a conversion pattern for the parallel Operation. This will help the conversion of parallel operation with standard dialect to parallel operation with llvm dialect. The type conversion of the block arguments in a parallel region is controlled by the pattern for the parallel Operation. Without this pattern, a parallel Operation with block arguments cannot be converted from standard to LLVM dialect. Other OpenMP operations without regions are marked as legal. When the translation of new OpenMP operations with regions are added then patterns for these operations can also be added.
This conversion uses all the standard to llvm patterns. Patterns of other dialects can be added later if needed.

See discussion in https://llvm.discourse.group/t/about-using-omp-dialect-in-mlir/1518/9

Diff Detail

Event Timeline

kiranchandramohan requested review of this revision.Aug 20 2020, 2:46 AM
flaub added a subscriber: flaub.Aug 20 2020, 3:11 PM
kiranchandramohan edited the summary of this revision. (Show Details)Aug 24 2020, 12:28 AM
kiranchandramohan edited reviewers, added: SouraVX, rriddle; removed: jdoerfert.
rriddle added inline comments.Aug 24 2020, 10:40 PM
mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
12

Are these includes necessary? I would expect that forward declarations would be enough.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
17

Can you trim these includes?

26

Classes should be placed in an anonymous namespace.

37

Did you intend to convert the result types here? Did you intend to pass in the converted operands, in operands as well?

39

The last .getOperation() shouldn't be necessary.

42

nit: Drop trivial braces.

mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
4

This looks like it is testing something already tested by the StandardToLLVM conversion pass. Can you trim this test down to checking what the patterns that you have added?

kiranchandramohan marked 4 inline comments as done.
kiranchandramohan marked 2 inline comments as done.

Addressed comments from @rriddle.

mlir/include/mlir/Conversion/OpenMPToLLVM/ConvertOpenMPToLLVM.h
12

Removed these but had to add <memory> for the unique_ptr

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
37

The result types are actually empty for the Parallel operation. So I have changed it here.

I saw that the operands of the parallel region are already converted so i did not want to do that in the conversion pattern also. This conversion pattern is only for converting the block arguments inside the parallel region to LLVM.
My understanding here might not be fully correct. If so please let me know if the operands of the parallel region also should be converted.

mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
4

I only have one function in the test now.
The conversion pattern is necessary for changing block arguments to llvm. So the test checks the block arguments and its uses inside the openmp region.

kiranchandramohan marked an inline comment as done.

Resubmitting with full context.

rriddle accepted this revision.Aug 26 2020, 1:16 AM
rriddle added inline comments.
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
26

nit: I'd just remove this, there isn't much of a need to protect it here.

37

In general, you should always use the updated operands unless you explicitly don't want to. When type conversions are involved the conversion infra won't implicitly replace operands with new values of a different type, so you should make sure to always use the updated operands in your patterns.

mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
4

nit: Can you add a CHECK-LABEL here?

This revision is now accepted and ready to land.Aug 26 2020, 1:16 AM
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
37

Thanks @rriddle. Is there a single function which can convert all the operands (from standard to llvm) or do i have to individually for each of the operands. Here some of the operands are attributes. What would be a good reference code?

rriddle added inline comments.Aug 26 2020, 7:00 AM
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
37

Is there a single function which can convert all the operands (from standard to llvm) or do i have to individually for each of the operands.

The converted operands are already being passed to the pattern via the operands parameters. This parameter contains the values that replaced the operands of the original operation, with the order corresponding 1-1 with the order of the original operand list. You can use the omp::ParallelOp::Adaptor class to provide named accessors(just like the proper omp::ParallelOp class) into this remapped operand list.

Here some of the operands are attributes.

nit on terminology, operands are never attributes. An argument, i.e. the thing you specify in ODS/Tablegen, may be either an Attribute or an Operand but these two are completely different concepts. An Operand is an SSA value, where as an Attribute is constant metadata.

What would be a good reference code?

Any of the other Foo(e.g. Standard)->LLVM conversion patterns are a good reference for LLVM conversions. For patterns in general, a lot of things in Conversion/ can be useful.

kiranchandramohan marked 5 inline comments as done.

Addressed comments from @rriddle.

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
37

Thanks for the explanation. I am using the converted operands passed to the pattern to create the new Operation as follows.

auto newOp = rewriter.create<omp::ParallelOp>(
    curOp.getLoc(), ArrayRef<Type>(), operands, curOp.getAttrs());

Thanks for the correction about the attributes and operands!