Page MenuHomePhabricator

[MLIR][OpenMP] Added omp.atomic.update
ClosedPublic

Authored by shraiysh on Nov 1 2021, 10:28 PM.

Details

Summary

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

Diff Detail

Event Timeline

shraiysh created this revision.Nov 1 2021, 10:28 PM
shraiysh requested review of this revision.Nov 1 2021, 10:28 PM
shraiysh retitled this revision from [MLIR][OpenMP] Added omp.atomic.update This patch supports the atomic construct (update) following section 2.17.7 of OpenMP 5.0 standard. Also added tests and verifier for the same. to [MLIR][OpenMP] Added omp.atomic.update.
shraiysh edited the summary of this revision. (Show Details)
peixin added inline comments.Nov 7 2021, 7:38 PM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
604

According to OpenMP 5.0 Spec 2.17.7 at page 239, operator is one of +, *, -, /, .AND., .OR., .EQV., or .NEQV.. I think you referred to C/C++ operator for XOR, SHIFTR and SHIFTL?

MAX, MIN is instrinsic procedure name, not operator here. What you plan to do is to use the format x = x max y in MLIR for x = max(x, y) in fortran code, right? If yes, did you miss IAND, IOR, IEOR?

I think BINOP is not the right term since MAX and MIN is intrinsic procedure name instead of binary operator. You should give a new name here.

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

How do you plan to handle the case x = max(x, max(y), min(z)) in fortran code? To make it as x = x max expr (lowering max(max(y), min(z)) to expr) in MLIR?

1482

In the else branch, the LHS X is also in RHS. isXLHSInRHSPart -> isXInRHSLeft? Or you can give one better name.

Thanks for the comments @peixin! I have tried to address them all, and will include the comments as mentioned if it is okay with you.

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

According to OpenMP 5.0 Spec 2.17.7 at page 239, operator is one of +, *, -, /, .AND., .OR., .EQV., or .NEQV.. I think you referred to C/C++ operator for XOR, SHIFTR and SHIFTL?

Yes, that is correct.

MAX, MIN is instrinsic procedure name, not operator here. What you plan to do is to use the format x = x max y in MLIR for x = max(x, y) in fortran code, right? If yes, did you miss IAND, IOR, IEOR?

The AND, OR and XOR are bitwise operations (will mention this as a comment for clarification). As logical values are essentially one-bit integer values, "logical and" is equivalent to "bitwise and for 1-bit integers". So we do not need both the sets AND, OR, XOR and IAND, IOR, IEOR.

I think BINOP is not the right term since MAX and MIN is intrinsic procedure name instead of binary operator. You should give a new name here.

I have borrowed the naming from OpenMP IR Builder (link). Although they do not handle MAX, MIN in createAtomicUpdate (link) right now, they have modeled it as a binary operation in llvm::AtomicRMWInst::BinOp (link). This is in line with C/C++, and prevents us from handling Fortran-specific features (intrinsic) in the common dialect. I am open to suggestions on another more appropriate naming though.

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

Yes, more or less. The evaluation of the expr_list (e1, e2, ...) in max(x, e1, e2, ... ) is not atomic. Hence, all such operations can be lowered to the following before OpenMPDialect:

expr = max(e1, e2... )
x = x max expr // atomic update

Let me know if the lowering doesn't seem right and breaks any concurrency related OpenMP guarantees.

1482

Again, this naming is borrowed from OpenMP IRBuilder. I was a little hesitant while using this variable too as the naming was not very clear. What the name is trying to say is "is X an LHS op in the RHS part?". I will put this as a comment here, but I have to agree I could not think of a better name. Any suggestions on this are welcome 😄

peixin accepted this revision.Nov 8 2021, 12:02 AM

Except for the variable name isXLHSInRHSPart, LGTM.

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

According to OpenMP 5.0 Spec 2.17.7 at page 239, operator is one of +, *, -, /, .AND., .OR., .EQV., or .NEQV.. I think you referred to C/C++ operator for XOR, SHIFTR and SHIFTL?

Yes, that is correct.

MAX, MIN is instrinsic procedure name, not operator here. What you plan to do is to use the format x = x max y in MLIR for x = max(x, y) in fortran code, right? If yes, did you miss IAND, IOR, IEOR?

The AND, OR and XOR are bitwise operations (will mention this as a comment for clarification). As logical values are essentially one-bit integer values, "logical and" is equivalent to "bitwise and for 1-bit integers". So we do not need both the sets AND, OR, XOR and IAND, IOR, IEOR.

I think BINOP is not the right term since MAX and MIN is intrinsic procedure name instead of binary operator. You should give a new name here.

I have borrowed the naming from OpenMP IR Builder (link). Although they do not handle MAX, MIN in createAtomicUpdate (link) right now, they have modeled it as a binary operation in llvm::AtomicRMWInst::BinOp (link). This is in line with C/C++, and prevents us from handling Fortran-specific features (intrinsic) in the common dialect. I am open to suggestions on another more appropriate naming though.

604

The this is fine with me. Thanks for the explanations.

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

I think it's viable to do this when lowering from parse-tree to MLIR.

1482

Well. I think the variable name should be changed together with the one in OpenMPIRBuilder since its name is not what it means. Maybe isXBinopExpr?
true if X is Left H.S. in Right H.S. part of the update expression, false otherwise. (e.g. true for X = X BinOp Expr) -> true for X = X BinOp Expr, and false for X = Expr BinOp X. (In OpenMPIRBuilder)
You may wait for better suggestions from others.

This revision is now accepted and ready to land.Nov 8 2021, 12:02 AM
shraiysh updated this revision to Diff 388215.Nov 18 2021, 8:31 AM

Address comments and rebase with main

Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2021, 8:31 AM
shraiysh updated this revision to Diff 388216.Nov 18 2021, 8:35 AM

Removed extra return in one test case (was added by mistake during rebase)

shraiysh updated this revision to Diff 388395.Nov 18 2021, 10:36 PM

Changed argument name in comments and arguments. This is ready for review now.

Thanks for this patch. Had a quick look. Looks OK. I have a couple of points.

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

Move to verifier (also)?

1466–1469

Should this be moved to the verifier (also)?

mlir/test/Dialect/OpenMP/ops.mlir
538–544

These could be for integers as well right? Can you add a test for regular integer types also?

shraiysh updated this revision to Diff 389939.Nov 26 2021, 1:35 AM

Rebase with main. Addressed comments.

shraiysh updated this revision to Diff 389940.Nov 26 2021, 1:36 AM

Rebase with main, addressed comments

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

I believe this cannot be moved to the verifier, as it's value is used in the next line (while making the attribute)

1466–1469

This cannot be moved to the verifier because expr is used in the resolveOperand call later and it cannot be null.

mlir/test/Dialect/OpenMP/ops.mlir
538–544

This was a mistake. It was supposed to be a test for regular integers and I don't think it makes sense to right shift on one-bit integers. I have modified it for i32 now.

shraiysh updated this revision to Diff 389942.Nov 26 2021, 1:39 AM

Modified the other test case for shiftr too (missed in previous diff)

shraiysh updated this revision to Diff 390623.Nov 30 2021, 1:45 AM

Rebase with main

What will prevent the builder from building operations which has the errors that you are catching during parsing?

Thanks for the review @kiranchandramohan.

What will prevent the builder from building operations which has the errors that you are catching during parsing?

The BinOp error will be taken care of by the AtomicBinOpKindAttr. An integer value from the allowed ones will be forced because of that constraint.

The other error - about X being in RHS - that is taken care of by design as the in memory structure has X, expr, binOp and isXBinOpExpr. It is not possible to run into that error while building the operation.

Thanks for the review @kiranchandramohan.

What will prevent the builder from building operations which has the errors that you are catching during parsing?

The BinOp error will be taken care of by the AtomicBinOpKindAttr. An integer value from the allowed ones will be forced because of that constraint.

The other error - about X being in RHS - that is taken care of by design as the in memory structure has X, expr, binOp and isXBinOpExpr. It is not possible to run into that error while building the operation.

Thanks. I have one more question regarding binary ops.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
597–636

Nit: Please add a TODO here to autogenerate from OMP.td in future if possible.

627

Are any non-binary operations supported currently? If not, can this be removed? If yes, can a test be added?

shraiysh updated this revision to Diff 390661.Nov 30 2021, 4:02 AM

Addressed Comments.

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

This is the naming issue from earlier. The name here means "is it of the form X binop expr". The value false implies that it is of the form expr binop X. The previous name for this variable was isXLhsinRhsPart but that was confusing too. This is not a flag for it being a binop expression or not.

kiranchandramohan accepted this revision.Dec 7 2021, 3:17 PM

Thanks for the clarification. LGTM.

shraiysh updated this revision to Diff 393037.Dec 8 2021, 10:52 PM

Thanks for the review. Rebase with main.

This revision was automatically updated to reflect the committed changes.