This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Add conversion from AtomicRMWOp -> GenericAtomicRMWOp.
ClosedPublic

Authored by pifon2a on Apr 23 2020, 1:20 PM.

Details

Summary

Adding this pattern reduces code duplication. There is no need to have a custom implementation for lowering to llvm.cmpxchg.

Diff Detail

Event Timeline

pifon2a created this revision.Apr 23 2020, 1:20 PM
Herald added a project: Restricted Project. · View Herald Transcript
pifon2a edited the summary of this revision. (Show Details)Apr 23 2020, 1:22 PM

Thanks for cleaning this up!

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2774

I would drop this. The pattern already checks for two specific operations. You can make it an assert if you want to ensure that the definition of matchSimpleAtomicOp and this one stay in sync.

Also, this should be in ConvertStandardToStandard (which is possible without the dependency on matchSimpleAtomicOp).

2780

This pattern does not have a test.

Also, maybe call it AtomicMinfMaxfTo... as that is what it is doing.

2955

Expose this from a the ConvertStandardToStandard header file with a descriptive name.

ftynse added inline comments.Apr 24 2020, 6:03 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2955

Yes, I was thinking that this pattern is not at all concerned with LLVM and should not live in "StandardToLLVM". However, the decision when to apply this pattern is driven by what LLVM supports.

Also, ConvertStandardToStandard feels weird to me. lib/Conversion was originally intended to solve the problem of the A->B conversion depending both on A dialect and B dialect so making little sense in either Dialect/A or Dialect/B... I would rather have it in lib/Dialect/StandardOps/Transforms/ExpandAtomic and call it from the LLVM conversion if necessary. If we ever have non-LLVM users or want to use the expansion for other cases than we currently do, it will be easier to extend.

herhut added inline comments.Apr 27 2020, 4:16 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2955

Thinking of it, we do the same in the GPU dialect where we have the AllReduceOpLowering that also lives in Transforms. So +1.

pifon2a updated this revision to Diff 261832.May 4 2020, 8:38 AM

Addressed the comments.

pifon2a marked 7 inline comments as done.May 4 2020, 8:43 AM
pifon2a added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2780

It was tested through lowering to cmpxchg. Now there is a separate test for this very pattern.

2955

Should we also clean-up mlir/lib/Conversion/StandardToStandard/StandardToStandard.cpp and move it to StandardOps/Transforms?

pifon2a updated this revision to Diff 262013.May 4 2020, 11:30 PM
pifon2a marked an inline comment as done.

updated the comment.

herhut accepted this revision.May 5 2020, 1:01 AM

Nice! Thanks.

This revision was not accepted when it landed; it landed in state Needs Review.May 5 2020, 1:34 AM
This revision was automatically updated to reflect the committed changes.