This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpenMP] Change omp.atomic.update to have generic updates
ClosedPublic

Authored by shraiysh on Feb 10 2022, 10:29 PM.

Details

Summary

This patch changes the syntax of omp.atomic.update to allow the other
dialects to modify the variable with appropriate operations in the
region.

Diff Detail

Event Timeline

shraiysh created this revision.Feb 10 2022, 10:29 PM
shraiysh requested review of this revision.Feb 10 2022, 10:29 PM
shraiysh updated this revision to Diff 408238.Feb 12 2022, 11:28 PM

llvm.intr.maxnum(f32) -> llvm.intr.smax(i32) in test case and rebase with main.

ftynse added a subscriber: ftynse.Feb 13 2022, 3:06 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
347–348

So this just dropped the type-match requirement for WsLoop and ReductionDeclare, hopefully only in documentation. In this case, the very least you could do is to move this documentation into that of the other ops that care. Or add "except for AtomicUpdate where it must match <...>" to this, but don't just drop important information on the floor.

672–675

Nit: I see that it was already there, but I would avoid such an non-specific reference to the standard. It's unclear which version of the standard this refers to, and there is no indication to anything more concrete than "the standard" that is hundreds of pages long. It is also subject to change.

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

Nit: the "pointer to region argument" is confusing. Consider something like "the type of the operand must be a pointer type whose element type is the same as that of the region argument", maybe you can find something more concise.

ftynse added inline comments.Feb 13 2022, 3:13 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1567

There doesn't seem to be a check for the region to have at least one argument, so getArgument(0) may assert.

shraiysh updated this revision to Diff 408269.Feb 13 2022, 8:32 AM
shraiysh marked 4 inline comments as done.

Thanks for the review @ftynse. Addressed comments.

shraiysh added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
347–348

Sure, added to the ReductionDeclareOp. WsLoopOp yields without any operands.

672–675

Added standard and section number. A one-line description of x is also added.

ftynse accepted this revision.Feb 15 2022, 3:36 AM
ftynse added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1568

Could we have a test for this? It can be written using the generic syntax.

This revision is now accepted and ready to land.Feb 15 2022, 3:36 AM
shraiysh updated this revision to Diff 408813.Feb 15 2022, 4:11 AM
shraiysh marked an inline comment as done.

Thanks for the comment @ftynse. Added testcase.