This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add polynomial approximation for math::LogOp
AbandonedPublic

Authored by ezhulenev on Feb 21 2021, 6:48 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 21 2021, 6:48 AM
ezhulenev requested review of this revision.Feb 21 2021, 6:48 AM
ezhulenev updated this revision to Diff 325301.Feb 21 2021, 7:01 AM

Use explisit SiToFp cast operation

ezhulenev edited the summary of this revision. (Show Details)Feb 21 2021, 7:22 AM
ezhulenev added a reviewer: asaadaldien.
mehdi_amini added inline comments.Feb 21 2021, 12:45 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I'm not sure it is a great thing to "re-invent" a new custom builder for the sake of a single pass: I'd like River/Alex to have another look.

326

Can we document the polynomial expansion? Or link to a documentation? (I don't find it on this link, I just found the raw source code).

ezhulenev added inline comments.Feb 21 2021, 1:13 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

The goal here is to create concise API that looks and feels like Eigen (approximations will come from Eigen initially). It is a ton of complexity in approximations itself, so it is nice to hide builders boilerplate and focus on numerics.

mehdi_amini added inline comments.Feb 21 2021, 6:56 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I sympathize, but I favor consistency in the MLIR project in itself in general. And having different ad-hoc builders or custom eDSLs that become pass specific is going against it.
That's why I'm looking for second opinions here :)

ezhulenev added inline comments.Feb 22 2021, 4:10 AM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I'm also a big fan of consistency :) In this case I think that's petty much the final state of this DSL, because all approximations are built from add/mul/cast/shift, and that's it. Also Rasmus is working on improving approximations in Eigen, and because he is the only expert I know of we have around, I hope to lure him into contributing to MLIR, and want to make this process as frictionless as possible.

asaadaldien added inline comments.Feb 22 2021, 2:36 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

Thanks Eugene for starting this, definitely better place to upstream some of IREE's fast math (e.g: https://github.com/google/iree/blob/main/iree/compiler/Conversion/LLVMToLLVM/FastExpConversion.cpp) .

Regarding Mehdi's custom builder point and looking at the log-approximation, Did you consider just using EDSC builders for each approximation ? llvm_madd, llvm_select...etc EDSC intrinsics that can be used and hides the builder and looks like the current API/DSL.

rriddle added inline comments.Feb 22 2021, 2:42 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I would say that some of the helpers seem useful, e.g. the ones that create a complex chain of operations. On the other hand, all of the methods that are one-to-one mappings do not really provide any value IMO. I don't think we should be creating these kinds of APIs.

I would also prefer that we do not let EDSCs creep in here.

mehdi_amini added inline comments.Feb 22 2021, 2:45 PM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

Regarding Mehdi's custom builder point and looking at the log-approximation, Did you consider just using EDSC builders for each approximation ?

EDSC is somehow in the same category, and kind of deprecated upstream at this point.

ezhulenev added inline comments.Feb 23 2021, 9:58 AM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I've implemented this patch using builder API in https://reviews.llvm.org/D97304, IMHO it is much harder to grasp what's going on behind all the builder/rewriter boilerplate, approximations themself are kind of ok, but frexp implementation for example is just lost in all these .create<....>(loc, ... over and over.

rriddle added inline comments.Feb 23 2021, 10:18 AM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96
mehdi_amini added inline comments.Feb 23 2021, 11:11 AM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I'd really like for us to have builders that are making the code easier to read, it has been hard to make progress on this in the past, but more feedback and example can help. Would you mind providing feedback (and point to this revision) here: https://llvm.discourse.group/t/rfc-dsl-style-dialect-specific-builders/1823 ?

ezhulenev added inline comments.Feb 23 2021, 11:52 AM
mlir/lib/Dialect/Math/Transforms/PolynomialApproximation.cpp
96

I've did another round with ImplicitLocOpBuilder and small number of helper functions, and now it looks reasonably good: https://reviews.llvm.org/D97304

ezhulenev abandoned this revision.Feb 24 2021, 10:46 AM