Page MenuHomePhabricator

[MLIR] Verify there are no side-effecting ops in GenericAtomicRMWOp body.
ClosedPublic

Authored by pifon2a on Apr 21 2020, 5:51 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Apr 21 2020, 5:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 5:51 AM
pifon2a updated this revision to Diff 258978.Apr 21 2020, 5:55 AM

Updated descrition in td.

pifon2a updated this revision to Diff 258979.Apr 21 2020, 5:56 AM

Fixed typo.

ftynse added a subscriber: ftynse.Apr 21 2020, 6:32 AM
ftynse added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
515

This looks like it also disallows ops with regions inside the atomic region. While I don't see an immediate use-case for it, I also don't think we should just ban them. Maybe walk the op instead and check for side effects?

pifon2a updated this revision to Diff 258998.Apr 21 2020, 7:39 AM

Address the comment.

pifon2a updated this revision to Diff 258999.Apr 21 2020, 7:40 AM

Remove whitespace.

herhut accepted this revision.Apr 21 2020, 12:44 PM
herhut added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
520

It would be extra nice if we could get a note where the side-effecting operation is. But that is icing on the cake.

This revision is now accepted and ready to land.Apr 21 2020, 12:44 PM
mehdi_amini added inline comments.Apr 21 2020, 8:46 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
520

Having the emitOpError on the operation inside the walk would probably get this achieved nicely I think?

pifon2a updated this revision to Diff 259178.Apr 22 2020, 12:03 AM
pifon2a marked 2 inline comments as done.

Addressed the comments.

This revision was automatically updated to reflect the committed changes.