[mlir][math] Set llvm readnone attribute for libm functions.

[vzakhari]

Summary

Math dialect operations currently do not limit transformations
applied to them, which means that they potentially behave like
clang's -ffast-math mathematics. Clang marks math functions with
readnone attribute enabling more optimizations.

This change does the same for functions used by MathToLibm convertor.
In particular, this enables LLVM LICM for tan() call in
Polyhedron/mp_prop_design_11 compiled with flang.

[ftynse]

mlir/lib/Conversion/MathToLibm/MathToLibm.cpp

138

passthrough is for hacks exploring performance-affecting attributes, we shouldn't enshrine it. We should rather model "readnone" properly.

[vzakhari]

mlir/lib/Conversion/MathToLibm/MathToLibm.cpp

138

Thank you for pointing out the hacky nature of passthrough, @ftynse! I was not aware of this.

Before I create a RFC on discourse, do you think it will be appropriate to add a readnone attribute for func::FuncOp? I suppose it should be tied to the SideEffect Interface implementation for func::FuncOp so that it reports no memory effects when readnone is present. Does it sound right?

[ftynse]

mlir/lib/Conversion/MathToLibm/MathToLibm.cpp

138

Yeah, that should have been better documented.

I am not entirely convinced that we want readnone as is on func.func. The viable approach is to have it as LLVM dialect attribute that can be attached to anything with FunctionOpInterface.

[vzakhari]

mlir/lib/Conversion/MathToLibm/MathToLibm.cpp

138

Thank you for the hint, Alex! D131457 should implement it the way you proposed - please let me know if you have any concerns.

[vzakhari]

Made use of llvm.readnone.

[vzakhari]

Added attribute checks for floor and ceil functions brought in by rebase.

[vzakhari]

Friendly ping. Please review.

[kiranchandramohan]

LGTM. Please wait a day before submission in case others have comments.

[kiranchandramohan]

I think you will need to add the LLVM as a dependentDialect in include/mlir/Conversion/Passes.td. The current entry (as shown below) does not include it.

def ConvertMathToLibm : Pass<"convert-math-to-libm", "ModuleOp"> {
  let summary = "Convert Math dialect to libm calls";
  let description = [{
    This pass converts supported Math ops to libm calls.
  }];
  let constructor = "mlir::createConvertMathToLibmPass()";
  let dependentDialects = [
    "arith::ArithmeticDialect",
    "func::FuncDialect",
    "vector::VectorDialect",
  ];
}

[vzakhari]

In D131031#3753493, @kiranchandramohan wrote:

I think you will need to add the LLVM as a dependentDialect in include/mlir/Conversion/Passes.td. The current entry (as shown below) does not include it.

def ConvertMathToLibm : Pass<"convert-math-to-libm", "ModuleOp"> {
  let summary = "Convert Math dialect to libm calls";
  let description = [{
    This pass converts supported Math ops to libm calls.
  }];
  let constructor = "mlir::createConvertMathToLibmPass()";
  let dependentDialects = [
    "arith::ArithmeticDialect",
    "func::FuncDialect",
    "vector::VectorDialect",
  ];
}

Thank you for the review!

As I unerstand, a dialect needs to be added as dependent only if it needs to be loaded in MLIRContext. It seems that just getting the attribute string name using LLVM::LLVMDialect::getReadnoneAttrName() does not need LLVM dialect to be loaded. llvm.readnone is just a named unit attribute, so it looks like using it also does not require LLVM dialect loading. I can easily add the dependency, I just think it will be a no-op.

[kiranchandramohan]

In D131031#3754273, @vzakhari wrote:

In D131031#3753493, @kiranchandramohan wrote:

I think you will need to add the LLVM as a dependentDialect in include/mlir/Conversion/Passes.td. The current entry (as shown below) does not include it.

def ConvertMathToLibm : Pass<"convert-math-to-libm", "ModuleOp"> {
  let summary = "Convert Math dialect to libm calls";
  let description = [{
    This pass converts supported Math ops to libm calls.
  }];
  let constructor = "mlir::createConvertMathToLibmPass()";
  let dependentDialects = [
    "arith::ArithmeticDialect",
    "func::FuncDialect",
    "vector::VectorDialect",
  ];
}

Thank you for the review!

As I unerstand, a dialect needs to be added as dependent only if it needs to be loaded in MLIRContext. It seems that just getting the attribute string name using LLVM::LLVMDialect::getReadnoneAttrName() does not need LLVM dialect to be loaded. llvm.readnone is just a named unit attribute, so it looks like using it also does not require LLVM dialect loading. I can easily add the dependency, I just think it will be a no-op.

Thanks for the explanation. When I went through the documentation it looked like it is required for attributes as well. If you are not seeing any issues then may be it is not required.
https://mlir.llvm.org/docs/PassManagement/#dependent-dialects

[vzakhari]

In D131031#3755286, @kiranchandramohan wrote:

In D131031#3754273, @vzakhari wrote:

In D131031#3753493, @kiranchandramohan wrote:

I think you will need to add the LLVM as a dependentDialect in include/mlir/Conversion/Passes.td. The current entry (as shown below) does not include it.

def ConvertMathToLibm : Pass<"convert-math-to-libm", "ModuleOp"> {
  let summary = "Convert Math dialect to libm calls";
  let description = [{
    This pass converts supported Math ops to libm calls.
  }];
  let constructor = "mlir::createConvertMathToLibmPass()";
  let dependentDialects = [
    "arith::ArithmeticDialect",
    "func::FuncDialect",
    "vector::VectorDialect",
  ];
}

Thank you for the review!

As I unerstand, a dialect needs to be added as dependent only if it needs to be loaded in MLIRContext. It seems that just getting the attribute string name using LLVM::LLVMDialect::getReadnoneAttrName() does not need LLVM dialect to be loaded. llvm.readnone is just a named unit attribute, so it looks like using it also does not require LLVM dialect loading. I can easily add the dependency, I just think it will be a no-op.

Thanks for the explanation. When I went through the documentation it looked like it is required for attributes as well. If you are not seeing any issues then may be it is not required.
https://mlir.llvm.org/docs/PassManagement/#dependent-dialects

I think the documentation might be talking about attributes that require MLIRContext for their creation, e.g. as in mlir/include/mlir/Dialect/LLVMIR/LLVMAttrDefs.td:

// Attribute definition for the LLVM Linkage enum.
def LinkageAttr : LLVM_Attr<"Linkage"> {
  let mnemonic = "linkage";
  let parameters = (ins
    "linkage::Linkage":$linkage
  );
  let hasCustomAssemblyFormat = 1;
}

This results in the following class structure:

class LinkageAttr : public ::mlir::Attribute::AttrBase<LinkageAttr, ::mlir::Attribute, detail::LinkageAttrStorage> {
public:
  using Base::Base;
  static LinkageAttr get(::mlir::MLIRContext *context, linkage::Linkage linkage);
  static constexpr ::llvm::StringLiteral getMnemonic() {
    return {"linkage"};
  }

  static ::mlir::Attribute parse(::mlir::AsmParser &odsParser, ::mlir::Type odsType);
  void print(::mlir::AsmPrinter &odsPrinter) const;
  linkage::Linkage getLinkage() const;
};

"Normal" way for creating such an attribute is to call LinkageAttr::get, which requires MLIRContext. At this point there will be verification that LLVM dialect is loaded. For example, I had to add the missing LLVM dialect dependency in D131538.

It seems that the way llvm.readnone is implemented does not require loading LLVM dialect.

[kiranchandramohan]

OK. LGTM.

mlir/lib/Conversion/MathToLibm/MathToLibm.cpp

ScalarOpToLibmCall<Op>::matchAndRewrite(Op op,
   if (!opFunc) {
     OpBuilder::InsertionGuard guard(rewriter);
     rewriter.setInsertionPointToStart(&module->getRegion(0).front());
     auto opFunctionTy = FunctionType::get(
         rewriter.getContext(), op->getOperandTypes(), op->getResultTypes());
     opFunc = rewriter.create<func::FuncOp>(rewriter.getUnknownLoc(), name,
                                            opFunctionTy);
     opFunc.setPrivate();
+
+    // By definition Math dialect operations imply LLVM's "readnone"
+    // function attribute, so we can set it here to provide more
+    // optimization opportunities (e.g. LICM) for backends targeting LLVM IR.
+    // This will have to be changed, when strict FP behavior is supported
+    // by Math dialect.
+    opFunc->setAttr("passthrough", rewriter.getArrayAttr(StringAttr::get(
+                                       rewriter.getContext(), "readnone")));
   }
   assert(isa<FunctionOpInterface>(SymbolTable::lookupSymbolIn(module, name)));
 
   rewriter.replaceOpWithNewOp<func::CallOp>(op, name, op.getType(),
                                             op->getOperands());
 
   return success();
 }

mlir/test/Conversion/MathToLibm/convert-to-libm.mlir

 // RUN: mlir-opt %s -convert-math-to-libm -canonicalize | FileCheck %s
 
-// CHECK-DAG: @atan(f64) -> f64
-// CHECK-DAG: @atanf(f32) -> f32
-// CHECK-DAG: @erf(f64) -> f64
-// CHECK-DAG: @erff(f32) -> f32
-// CHECK-DAG: @expm1(f64) -> f64
-// CHECK-DAG: @expm1f(f32) -> f32
-// CHECK-DAG: @atan2(f64, f64) -> f64
-// CHECK-DAG: @atan2f(f32, f32) -> f32
-// CHECK-DAG: @tan(f64) -> f64
-// CHECK-DAG: @tanf(f32) -> f32
-// CHECK-DAG: @tanh(f64) -> f64
-// CHECK-DAG: @tanhf(f32) -> f32
-// CHECK-DAG: @round(f64) -> f64
-// CHECK-DAG: @roundf(f32) -> f32
-// CHECK-DAG: @cos(f64) -> f64
-// CHECK-DAG: @cosf(f32) -> f32
-// CHECK-DAG: @sin(f64) -> f64
-// CHECK-DAG: @sinf(f32) -> f32
+// CHECK-DAG: @atan(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @atanf(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @erf(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @erff(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @expm1(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @expm1f(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @atan2(f64, f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @atan2f(f32, f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @tan(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @tanf(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @tanh(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @tanhf(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @round(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @roundf(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @cos(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @cosf(f32) -> f32 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @sin(f64) -> f64 attributes {passthrough = ["readnone"]}
+// CHECK-DAG: @sinf(f32) -> f32 attributes {passthrough = ["readnone"]}
 
 // CHECK-LABEL: func @atan_caller
 // CHECK-SAME: %[[FLOAT:.*]]: f32
 // CHECK-SAME: %[[DOUBLE:.*]]: f64
 // CHECK-SAME: %[[HALF:.*]]: f16
 // CHECK-SAME: %[[BFLOAT:.*]]: bf16
 func.func @atan_caller(%float: f32, %double: f64, %half: f16, %bfloat: bf16) -> (f32, f64, f16, bf16)  {
   // CHECK: %[[FLOAT_RESULT:.*]] = call @atanf(%[[FLOAT]]) : (f32) -> f32