Page MenuHomePhabricator

[MLIR] Add GenericAtomicRMWOp.
ClosedPublic

Authored by pifon2a on Apr 17 2020, 2:54 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Apr 17 2020, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2020, 2:54 AM
pifon2a updated this revision to Diff 258338.Apr 17 2020, 8:33 AM

Added block arg for the old value.

pifon2a updated this revision to Diff 258341.Apr 17 2020, 8:41 AM

Added `getCurrentValue() method and updated the doc.

pifon2a updated this revision to Diff 258348.Apr 17 2020, 9:11 AM

Remove whitespace in GenericAtomicRMW printer.

Thanks. This looks great to me. Has there been consensus on https://llvm.discourse.group/t/rfc-add-std-atomic-rmw-op/489 yet? There were no more replies but it seems nobody is opposed either.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
507

x -> %x

mlir/test/IR/invalid-ops.mlir
1171

Could you add a test for the case where the memref and result types disagree?

pifon2a updated this revision to Diff 258716.Apr 20 2020, 6:17 AM

Addressed the comments.

pifon2a marked 2 inline comments as done.Apr 20 2020, 6:18 AM

Thanks. This looks great to me. Has there been consensus on https://llvm.discourse.group/t/rfc-add-std-atomic-rmw-op/489 yet? There were no more replies but it seems nobody is opposed either.

I implemented exactly what we agreed to with flaub@. There were no additional replies after that.

herhut accepted this revision.Apr 20 2020, 8:01 AM

Thanks for adding this!

This revision is now accepted and ready to land.Apr 20 2020, 8:01 AM
mehdi_amini added inline comments.Apr 20 2020, 9:10 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
502

We should mention that no side effecting operation is allowed in the region.

536

This is the kind of method I expect provided by a Traits otherwise we're losing consistency across all operations with regions.

flaub accepted this revision.Apr 20 2020, 11:10 AM

LGTM

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
511

Could we add a check that all the ops in the region are side-effect free?

herhut added inline comments.Apr 21 2020, 1:51 AM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
536

Do you have a new trait in mind or should this be supported by the SingleBlockImplicitTerminator trait? The body builder makes only sense if there is a single region.

These are somewhat widespread already. I'd rather have it as a separate cleanup.

pifon2a marked 5 inline comments as done.Apr 21 2020, 5:58 AM

Thanks !

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
502
536

I am happy to do the cleanup as soon as there is some consensus.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
511
mehdi_amini added inline comments.Apr 21 2020, 8:33 PM
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
536

Adding it to SingleBlockImplicitTerminator looks fine to me

pifon2a closed this revision.Apr 22 2020, 12:12 AM
pifon2a marked 3 inline comments as done.
pifon2a added inline comments.
mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
536

Will do.