This is an archive of the discontinued LLVM Phabricator instance.

[mlir] add support for reductions in OpenMP WsLoopOp
ClosedPublic

Authored by ftynse on Jul 2 2021, 9:30 AM.

Details

Summary

Use a modeling similar to SCF ParallelOp to support arbitrary parallel
reductions. The two main differences are: (1) reductions are named and declared
beforehand similarly to functions using a special op that provides the neutral
element, the reduction code and optionally the atomic reduction code; (2)
reductions go through memory instead because this is closer to the OpenMP
semantics.

See https://llvm.discourse.group/t/rfc-openmp-reduction-support/3367.

Diff Detail

Event Timeline

ftynse created this revision.Jul 2 2021, 9:30 AM
ftynse requested review of this revision.Jul 2 2021, 9:30 AM

Thanks @ftynse for this patch. I am just making my way through this patch. Should be able to spend more time on this later today. Have a few micro-nits and questions to start.

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

We discussed this in the RFC, but will this dependency cause issues for any future non-llvm or out of tree lowerings?

41

Nit: Spellings.

177

Nit: Something missing at the start of this line.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
50–52

Do we have to do this attachInterface for FIR or other dialect types? Or can we use OpenMP_PointerLikeTypeInterface while declaring the FIR types?

780–793

This looks trivial. Why is a custom parser and printer required here?

mlir/test/Dialect/OpenMP/invalid.mlir
108

Nit: Missing last character.

138

Nit: Missing last character.

154

Nit: will be good to have "expects" in the beginning of the expected error.

A couple more questions.

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

Nit: spelling requies

391

Will this region finally sit outside the worksharing loop?

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

Should there be a check to ensure that the operand type is the same as the element type of the accumulator?

ftynse updated this revision to Diff 357166.Jul 8 2021, 2:01 AM
ftynse marked 11 inline comments as done.

Address review.

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

Absolutely not. And this dependency already exists, as you may note I'm not modifying the cmake. This only indicates to the infrastructure that this dialect might produce objects of the LLVM dialect.

391

This is a reduction _declaration_ (2.19.5.7), it has an initializer that is independent on the value that the accumulator has at the start of the workshare loop.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
50–52

You can do both as long as you don't do it in the main codebase. This was the point of separable registration for interfaces -- OpenMP won't have to know about FIR. Optionally, we can also have FIR not know about OpenMP and have a completely separate registration, but it should not be the primary option.

780–793

One cannot use a keyword as anchor for an optional group in the declarative format.

854

This is already checked by TypeMatchesWith in ODS specification of the op.

One more question to clarify the dependency.

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

The current dependency is only during translation I think. Previously we had the llvm integer type as an option for the worksharing loop indices, but that is also not there.

But if we choose the LLVM token type we now have a hard dependency isn't it?

ftynse marked an inline comment as done.Jul 8 2021, 6:03 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
27

There is no token type in this version, it is unnecessary with the "variable" approach. To support the use of LLVM pointer types for variables, OpenMP must be aware of the LLVM dialect since we don't want LLVM to depend on OpenMP.

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

We have this code here.

ftynse updated this revision to Diff 357219.Jul 8 2021, 7:24 AM
ftynse marked 2 inline comments as done.

Drop leftover code.

ftynse added inline comments.Jul 8 2021, 7:24 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
935

Which is a leftover of a previous version. Thanks, this is a nice catch!

kiranchandramohan accepted this revision.Jul 8 2021, 4:17 PM

Reduction is an important clause in the specification. This is also a very good reference for us to make further progress in other constructs. Thanks, @ftynse.

LGTM.

This revision is now accepted and ready to land.Jul 8 2021, 4:17 PM
This revision was automatically updated to reflect the committed changes.