Page MenuHomePhabricator

[mlir] Add patterns to lower Math operations to LLVM based libm calls.
ClosedPublic

Authored by tpopp on Apr 13 2021, 1:21 AM.

Details

Summary

Some Math operations do not have an equivalent in LLVM. In these cases,
allow a low priority fallback of calling the libm functions. This is to
give functionality and is not a performant option.

Diff Detail

Event Timeline

tpopp created this revision.Apr 13 2021, 1:21 AM
tpopp requested review of this revision.Apr 13 2021, 1:21 AM
ftynse added inline comments.Apr 13 2021, 9:38 AM
mlir/lib/Conversion/MathToLLVM/MathToLibm.cpp
26 ↗(On Diff #337060)

Nit: I would consider exposing benefit as a constructor argument, and further exposing it at populateMathToLibmConversionPatterns. Thus, the caller will be able to set up the benefit, low or high, depending on their needs.

51 ↗(On Diff #337060)

Unused variable.

61 ↗(On Diff #337060)

I expect LLVM::detail::vectorOneToOneRewrite to be able to handle this, maybe with minor extension. It already handles "standard" ops on vector types.

84 ↗(On Diff #337060)

Consider turning this into an OpConversionPattern or ConvertOpToLLVMPattern that carries a type converter with it instead of creating it locally. It makes it clear that some type conversion is happening within the pattern, and will also work with customized type converters.

87 ↗(On Diff #337060)

Nit: sink this to the actual definition place

88 ↗(On Diff #337060)

Please use isa<Float32Type, Float64Type>. The code below assumes the value is either float or double, but we support other floating-point types.

95 ↗(On Diff #337060)

Consider asserting that the return types match instead. Otherwise this code may be creating a duplicate symbol (function).

On a second thought, does this even need to target LLVM dialect directly? I'd expect us to have vector insert/extract in the vector dialect (hopefully, there is also a facility to unpack vectors; I see a naive combination of vector-to-loops and loop-unroll, but there may be something more specific) and we can call functions at the std level. The std->llvm conversion will do the rest.

tpopp marked 7 inline comments as done.Apr 14 2021, 6:37 AM

On a second thought, does this even need to target LLVM dialect directly? I'd expect us to have vector insert/extract in the vector dialect (hopefully, there is also a facility to unpack vectors; I see a naive combination of vector-to-loops and loop-unroll, but there may be something more specific) and we can call functions at the std level. The std->llvm conversion will do the rest.

This is a good point. Because libm is not available to all backends, I was focused on LLVM, but this is still true for backends to LLVM, so that argument does not make sense. I changed this to go to Std function calls. I have not been able to find vector dialect ops to pack and unpack values from a vector though unfortunately.

mlir/lib/Conversion/MathToLLVM/MathToLibm.cpp
61 ↗(On Diff #337060)

With your point that this doesn't need to require LLVM, this is no longer as similar, so I would say it's obsolete.

84 ↗(On Diff #337060)

Obsolete based on your later suggestion.

tpopp marked 2 inline comments as done.Apr 14 2021, 6:37 AM
tpopp updated this revision to Diff 337440.Apr 14 2021, 6:53 AM

Convert to StandardOpsDialect instead of LLVMDialect.

Additionally, move/rename files to remove LLVM fully from being related to the conversion.

I have not been able to find vector dialect ops to pack and unpack values from a vector though unfortunately.

It may not exist, it just sounds like a useful thing to have so I thought somebody could have added it. Feel free to have a local version or suggest one at Vector level. There's already VectorUnrollPattern that could be helpful.

mlir/lib/Conversion/MathToLibm/MathToLibm.cpp
37

Nit: could we take StringRef here to avoid extra copies?

mlir/test/Conversion/MathToLLVM/convert-to-libm.mlir
2

Nit: do we need --debug for some reason here?

ftynse accepted this revision.Apr 16 2021, 8:55 AM
This revision is now accepted and ready to land.Apr 16 2021, 8:55 AM
tpopp updated this revision to Diff 338726.Apr 19 2021, 11:32 PM
tpopp marked 2 inline comments as done.

Add test case to convert vectors to scalars.

It may not exist, it just sounds like a useful thing to have so I thought somebody could have added it. Feel free to have a local version or suggest one at Vector level. There's already VectorUnrollPattern that could be helpful.

I'm going to leave this as local for now and follow up on a modification for VectorUnrollPattern or a separate pattern when I have more time.

mlir/test/Conversion/MathToLLVM/convert-to-libm.mlir
2

Removed