Page MenuHomePhabricator

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

Authored by vzakhari on Aug 2 2022, 4:34 PM.

Details

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.

Diff Detail

Event Timeline

vzakhari created this revision.Aug 2 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 4:34 PM
vzakhari requested review of this revision.Aug 2 2022, 4:34 PM
ftynse added inline comments.Aug 4 2022, 3:38 AM
mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
139

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

vzakhari added inline comments.Aug 4 2022, 4:12 PM
mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
139

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 added inline comments.Aug 5 2022, 1:22 AM
mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
139

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 added inline comments.Aug 8 2022, 6:01 PM
mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
139

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

vzakhari updated this revision to Diff 455284.Aug 24 2022, 10:48 AM

Made use of llvm.readnone.

vzakhari updated this revision to Diff 455285.Aug 24 2022, 10:50 AM

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

Friendly ping. Please review.

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

This revision is now accepted and ready to land.Aug 27 2022, 5:09 AM

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",
  ];
}

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.

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 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.