This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add lowering from math.expm1 to LLVM.
ClosedPublic

Authored by akuegel on Feb 16 2021, 5:40 AM.

Details

Summary

This is the straightforward lowering to exp(x) - 1.

Diff Detail

Event Timeline

akuegel created this revision.Feb 16 2021, 5:40 AM
akuegel requested review of this revision.Feb 16 2021, 5:40 AM
akuegel updated this revision to Diff 323974.Feb 16 2021, 5:55 AM

Fix comment that was still copy/pasted from rsqrt.

I won't ask to split out the math conversion from "standard" conversion, yet, but have you considered implementing this (as well as rsqrt from which most of the code is duplicated) as a sort of expand-math conversion instead?

herhut accepted this revision.Feb 24 2021, 12:24 AM

I won't ask to split out the math conversion from "standard" conversion, yet, but have you considered implementing this (as well as rsqrt from which most of the code is duplicated) as a sort of expand-math conversion instead?

Splitting this out is on my todo list. I though about having a general lowering to libm / llvm intrinsics that converts everything into a form that can be mapped that way. Additionally, we will have a lowering to GPU intrinsics and also the explicit approximations that are currently being worked on. Then the user of these lowerings can choose between those three sets of patterns, depending on their use-case. WDYT?

This revision is now accepted and ready to land.Feb 24 2021, 12:24 AM

Then the user of these lowerings can choose between those three sets of patterns, depending on their use-case. WDYT?

As long as the patterns are reusable so that the user can combine them and decide what is legal/illegal to chose a lowering strategy "per operation" :)

Sorry for not following up on this patch. I was very busy this week to work on a new op that we need for Kernel Generator broadcast specialization, and that is blocking and thus has priority over this patch.
I hope I can do the necessary adjustments to this patch soon.

cota added a comment.May 3 2021, 3:15 PM

Ping.
I just added log1p's approximation (https://reviews.llvm.org/D101765). I'd like this patch to go in so that I can also add expm1's approximation.

Ping.
I just added log1p's approximation (https://reviews.llvm.org/D101765). I'd like this patch to go in so that I can also add expm1's approximation.

Essentially this patch is blocked by same thing that was also commented on https://reviews.llvm.org/D98662, that we should not add additional copy/paste conversions here.
Personally I would be ok with landing this patch as-is to unblock you, if that is acceptable. But I am still working on other things which have a higher priority to my team than working on a cleanup here by adding a new math conversion pass.
@herhut Please correct me in case I got the prioritization wrong.

ftynse accepted this revision.May 4 2021, 4:46 AM

Like I said, I won't block this (and I haven't blocked https://reviews.llvm.org/D98662 either), but let's try and find time across teams to proceed with the refactoring -- it's not a priority for me either, but maintaining an ever-growing list of patterns may become hard.

Like I said, I won't block this (and I haven't blocked https://reviews.llvm.org/D98662 either), but let's try and find time across teams to proceed with the refactoring -- it's not a priority for me either, but maintaining an ever-growing list of patterns may become hard.

Maybe we should have a fixit week for such cleanups :)
And I also agree that a separate pass would be better (so that we would not have to deal with the "is it a vector type" separately in each such op, and instead can rely on that handling inside the lowering of the ops into which it is composed).

This revision was automatically updated to reflect the committed changes.