Page MenuHomePhabricator

[mlir] Add polynomial approximation for math::LogOp (using builders API)
ClosedPublic

Authored by ezhulenev on Feb 23 2021, 9:30 AM.

Details

Summary

Replace math::LogOp with an approximations from the the Julien Pommier's SSE math library

Link: http://gruntthepeon.free.fr/ssemath

Diff Detail

Event Timeline

ezhulenev created this revision.Feb 23 2021, 9:30 AM
ezhulenev requested review of this revision.Feb 23 2021, 9:30 AM

Use ImplicitLocOpBuilder

ezhulenev retitled this revision from [mlir] Use builder API to construct approximations to [mlir] Add polynomial approximation for math::LogOp (using builders API).Feb 23 2021, 11:51 AM
ezhulenev edited the summary of this revision. (Show Details)

Code LGTM, I didn't review carefully the numerics here, @asaadaldien is this something you'd like to look at?

asaadaldien accepted this revision.Feb 23 2021, 10:16 PM

LGTM, The log-approximation looks very similar to another implementation I am familiar with (https://github.com/halide/Halide/blob/master/src/IROperator.cpp#L1147)
Some (approximation precision - performance) trade off can be revisited later IMO.

mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
303

The shift adds more operations and still uses same 8-order polynomial as https://github.com/boulos/syrah/blob/master/src/include/syrah/FixedVectorMath.h#L460, I wonder does it add more precision? if yes Can we reach same precision as syrah's implementation with lower polynomial order ?

This revision is now accepted and ready to land.Feb 23 2021, 10:16 PM
asaadaldien added inline comments.Feb 23 2021, 10:20 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
146

abs isn't needed here if we are using a strictly positive frexp which is useful in the log(x), x > 0 we have below. This requires using a different cstInvMantMask see (https://github.com/boulos/syrah/blob/master/src/include/syrah/FixedVectorMath.h#L426)

ezhulenev updated this revision to Diff 326078.Feb 24 2021, 6:59 AM

Optimize frexp for positive values

ezhulenev added inline comments.Feb 24 2021, 7:50 AM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
146

Added a flag to disable abs, it saved ~5% of cpu time in benchmarks. Re mask: didn't quite get it, I think it is irrelevant, and 0x807FFFF is exactly the inverse of 0x7f800000.

303

Added a note to explore this option in the followup PRs.

This revision was landed with ongoing or failed builds.Feb 24 2021, 7:50 AM
This revision was automatically updated to reflect the committed changes.

Hi @ezhulenev!

It looks like elementType and isI32 methods in PolynomialApproximation.cpp are not used. Would it be possible to remove them to avoid the warnings?

Thanks!
Diego

They are actually used but only inside assert, is it a release build when it is a warning?

They are actually used but only inside assert, is it a release build when it is a warning?

Oh, yes, it looks like it's only for release build. Not sure about the guidelines for this case. We couldn't use #ifndef NDEBUG since asserts can also be enabled in release build. Could we introduce some dummy casting for them as we do for other variables that are only used inside asserts (https://llvm.org/docs/CodingStandards.html#id44)?

#ifndef NDEBUG works because this is exactly what is controlled by enabling/disabling assert in release mode.