Adding this pattern reduces code duplication. There is no need to have a custom implementation for lowering to llvm.cmpxchg.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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. |
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).