This is an archive of the discontinued LLVM Phabricator instance.

[mlir][OpenMP] Added omp.atomic.capture operation
ClosedPublic

Authored by shraiysh on Dec 15 2021, 10:52 PM.

Details

Summary

This patch supports the atomic construct (capture) following section 2.17.7 of OpenMP 5.0 standard. Also added tests for the same.

Diff Detail

Event Timeline

shraiysh created this revision.Dec 15 2021, 10:52 PM
shraiysh requested review of this revision.Dec 15 2021, 10:52 PM
peixin added inline comments.Dec 30 2021, 4:54 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
726

For fortran code, expr here refers to both expr and expr_list.

730
The evaluation of `expr` need not be atomic w.r.t the read or write of the location designated by `x`.

This is for update clause. For capture clause, the standard states that Neither the evaluation of expr or expr_list, nor the write to the location designated by v, need be atomic with respect to the read or write of the location designated by x.

730
In general, type(x) must dereference to type(expr).

Not clear what this means.

732

Not only binary operation. Since this is used for both c/c++ and fortran, binop is the binary operator or intrinsic (MAX, MIN, IAND, IOR, or IEOR).

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1536–1537

The restriction states that If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire, which is not for capture clause. One interesting thing is that the description in the standard states that If the write, update, or capture clause is specified and the release, acq_rel, or seq_cst clause is specified then the strong flush on entry to the atomic operation is also a release flush. Checked with OpenMP 5.1, and there is no this kind of restriction. Don't understand why acq_rel and acquire are not allowed in update or capture clause.

1549

Nit: How about x_type , v_type , expr_type?

1567

%x = %expr binop %x | %x = %x binop %expr would be clear than %x = ...

1601

The same as above.

1632

Nit: remove the brace.

1673
mlir/test/Dialect/OpenMP/ops.mlir
601

lines 593-602 ->

v = x = x binop expr;
x = x binop expr; v = x;
607
v = x = expr binop x;
x = expr binop x; v = x;
619

lines 613-620 ->

v = x; x = x binop expr;
v = x; x binop= expr;
633

Redundant test cases. Move the comment fortran expressions in front of the above test cases.

Thanks for the detailed review @peixin. I will work on this patch and address comments based on the discussion from patch D116396.

shraiysh updated this revision to Diff 397493.Jan 5 2022, 2:53 AM

Updated patch to use omp.atomic.read and omp.atomic.update

shraiysh marked 14 inline comments as done.Jan 5 2022, 3:08 AM
shraiysh updated this revision to Diff 397497.Jan 5 2022, 3:29 AM

Fix missing semi-colon error

peixin accepted this revision.Jan 6 2022, 12:49 AM

This design is much easier to review. Nice job. LGTM.

This revision is now accepted and ready to land.Jan 6 2022, 12:49 AM

For discussion,
-> Generally, in a single block region, if the terminator has no special semantics then it is better to use SingleBlockImplicitTerminator. In this case more so since it is just a couple of instructions.
-> Would it be better to model to capture x, expr, v, y etc as operands of the capture operation?
-> Would it be better to model this is a complex operation without a region?

peixin added a comment.Jan 6 2022, 3:38 AM

-> Would it be better to model to capture x, expr, v, y etc as operands of the capture operation?
-> Would it be better to model this is a complex operation without a region?

The first version of this patch model the capture construct using one standalone operation (https://reviews.llvm.org/D115851?id=394750), but it is kind of hard to read.

-> Would it be better to model to capture x, expr, v, y etc as operands of the capture operation?
-> Would it be better to model this is a complex operation without a region?

The first version of this patch model the capture construct using one standalone operation (https://reviews.llvm.org/D115851?id=394750), but it is kind of hard to read.

OK. The only concern I have is that the operands of the operation are not captured in the operation definition. I don't know whether that will cause any issues. I think we can go ahead with this for now. Can you address the terminator comment?

Thank you for the reviews @kiranchandramohan and @peixin. I will address the terminator comment soon. I have been busy with some other work recently. Apologies for the delay.

shraiysh updated this revision to Diff 399622.Jan 13 2022, 3:42 AM

Addressed SingleBlockImplicitTerminator comment.

Gentle ping!

  1. If the terminator is implicit will it create difficulties in lowering?
  2. Should we update the examples in the description and the tests to be without the terminator?
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1569

If the terminator is implicit, does this hold?

Thanks for the comments @kiranchandramohan. IIUC SingleBlockImplicitTerminator just adds an error message if the region does not end with omp.terminator. I thought it would accept the following code but it gives a missing omp.terminator error instead

omp.atomic.capture {
  omp.atomic.read ...
  omp.atomic.update ...
}

I think lowering should not be a problem because the in-memory structure still has an omp.terminator at the end of the region.

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

Yes, it still holds (there is a test for this error message). I expected that with an implicit terminator, it will only have two, but this is not true. It looks like having SingleBlockImplicitTerminator just makes sure that we end a region with omp.terminator.

Ahh, sorry about that. I kind of thought the terminator will not be visible.

LGTM.

shraiysh updated this revision to Diff 402492.Jan 24 2022, 5:45 AM

Thanks! Rebase with main.

This revision was automatically updated to reflect the committed changes.