Page MenuHomePhabricator

[MLIR] Lower GenericAtomicRMWOp to llvm.cmpxchg.
ClosedPublic

Authored by pifon2a on Apr 22 2020, 8:26 AM.

Details

Summary

Lowering is pretty much a copy of AtomicRMWOp -> llvm.cmpxchg
pattern.

Diff Detail

Event Timeline

pifon2a created this revision.Apr 22 2020, 8:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 8:26 AM

What should we do with the lowering for AtomicRMWOp "minf", "maxf"? Should we lower it to GenericAtomicRMWOp and then to LLVM? Or maybe we should not use AtomicRMWOp for these cases at all? It can always be replaced by GenericAtomicRMWOp.

OK, I could see always lowering enums that would lower to cmpxchg via the generic op. But at the moment we have a higher layer that uses the enumerated version for min/max, so I'd prefer to don't remove that. It's much easier to pattern match (even declaratively) the enumerated version than the one with a region.

flaub accepted this revision.Apr 22 2020, 11:27 AM
This revision is now accepted and ready to land.Apr 22 2020, 11:27 AM

@flaub Thanks, lowering of AtomicRMWOp -> GenericAtomicRMWOp then it is.

This revision was automatically updated to reflect the committed changes.
ftynse added a subscriber: ftynse.Apr 23 2020, 1:16 AM
ftynse added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2796

This is not allowed in conversion patterns (it is not undoable). I've recently introduced a functionality to create block in a rewriter-compatible way, rewriter.createBlock(initBlock->getParent(), std::next(Region::iterator(initBlock)), valueType); should work.

pifon2a marked an inline comment as done.Apr 23 2020, 2:04 AM
pifon2a added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2796

Thanks a lot, Alex. Good to know that it is not allowed. I ll send a patch to fix this.