[MLIR][Math] Fix mathtolibm to use conversion patterns

[wsmoses]

This leads to catastrophic failures, especially when using conversion patterns inside the greedy driver that doesn't provide sufficient rewriter infrastructure.

As this is in conversion (and exported in populate functions), this should be a conversion rather than rewriter. And indeed I do see a segfault due to a mismatch of rewrite/conversion in a downstream project.

[wsmoses]

x/ref #154075 EnzymeAD/Enzyme-JAX#1278

[llvmbot]

@llvm/pr-subscribers-mlir

Author: William Moses (wsmoses)

Changes

This leads to catastrophic failures, especially when using conversion patterns inside the greedy driver that doesn't provide sufficient rewriter infrastructure.

As this is in conversion (and exported in populate functions), this should be a conversion rather than rewriter. And indeed I do see a segfault due to a mismatch of rewrite/conversion in a downstream project.

---

Full diff: https://github.com/llvm/llvm-project/pull/154083.diff

1 Files Affected:

• (modified) mlir/lib/Conversion/MathToLibm/MathToLibm.cpp (+13-10)

diff --git a/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp b/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
index f7c0d4fe3a799..70514658ac366 100644
--- a/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
+++ b/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
@@ -29,32 +29,35 @@ namespace {
 // Pattern to convert vector operations to scalar operations. This is needed as
 // libm calls require scalars.
 template <typename Op>
-struct VecOpToScalarOp : public OpRewritePattern<Op> {
+struct VecOpToScalarOp : public OpConversionPattern<Op> {
 public:
-  using OpRewritePattern<Op>::OpRewritePattern;
+  using OpConversionPattern<Op>::OpConversionPattern;
 
-  LogicalResult matchAndRewrite(Op op, PatternRewriter &rewriter) const final;
+  LogicalResult
+  matchAndRewrite(Op op, ConversionPatternRewriter &rewriter) const final;
 };
 // Pattern to promote an op of a smaller floating point type to F32.
 template <typename Op>
-struct PromoteOpToF32 : public OpRewritePattern<Op> {
+struct PromoteOpToF32 : public OpConversionPattern<Op> {
 public:
-  using OpRewritePattern<Op>::OpRewritePattern;
+  using OpConversionPattern<Op>::OpConversionPattern;
 
-  LogicalResult matchAndRewrite(Op op, PatternRewriter &rewriter) const final;
+  LogicalResult
+  matchAndRewrite(Op op, ConversionPatternRewriter &rewriter) const final;
 };
 // Pattern to convert scalar math operations to calls to libm functions.
 // Additionally the libm function signatures are declared.
 template <typename Op>
-struct ScalarOpToLibmCall : public OpRewritePattern<Op> {
+struct ScalarOpToLibmCall : public OpConversionPattern<Op> {
 public:
   using OpRewritePattern<Op>::OpRewritePattern;
   ScalarOpToLibmCall(MLIRContext *context, PatternBenefit benefit,
                      StringRef floatFunc, StringRef doubleFunc)
-      : OpRewritePattern<Op>(context, benefit), floatFunc(floatFunc),
-        doubleFunc(doubleFunc) {};
+      : OpConversionPattern<Op>(context, benefit), floatFunc(floatFunc),
+        doubleFunc(doubleFunc){};
 
-  LogicalResult matchAndRewrite(Op op, PatternRewriter &rewriter) const final;
+  LogicalResult
+  matchAndRewrite(Op op, ConversionPatternRewriter &rewriter) const final;
 
 private:
   std::string floatFunc, doubleFunc;

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- mlir/lib/Conversion/MathToLibm/MathToLibm.cpp

View the diff from clang-format here.

diff --git a/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp b/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
index 3cbe0aa5f..21e35e119 100644
--- a/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
+++ b/mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
@@ -56,7 +56,7 @@ public:
   ScalarOpToLibmCall(MLIRContext *context, PatternBenefit benefit,
                      StringRef floatFunc, StringRef doubleFunc)
       : OpConversionPattern<Op>(context, benefit), floatFunc(floatFunc),
-        doubleFunc(doubleFunc){};
+        doubleFunc(doubleFunc) {};
 
   LogicalResult
   matchAndRewrite(Op op, typename OpConversionPattern<Op>::OpAdaptor adaptor,

[joker-eph]

Same request as the other PR: I'd like to see a description that refers to something principled into the design and constraint of the dialect conversion. Let's wait for @matthias-springer to help on this.

[matthias-springer]

Same as #154075.

[wsmoses]

okay figured out and fixedthe source of the segfault from our end which is indeed unrelated to this PR (essentially blockargument being added during conversion, outside of pattern infra). This still might be useful to do (will leave that discussion to you guys being more familiar with this part of the code)