This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyLibCalls] Add a new transform: tan(atan(x)) -> x
ClosedPublic

Authored by davide on Nov 3 2015, 1:22 PM.

Details

Summary

To put things into context, I did some basic research trying to understand which lib calls transformation gcc does but llvm does not under -ffast-math, and started writing code to reduce the difference. Probably I should've explained this in my previous pow() patch, but, anyway.

This is the second one of a series of local patches I'll try to submit over the next weeks. It may at this point considered a WIP (still need to add more comprehensive tests, among others). I'm not completely sure if we should remove the call to atan (I wasn't confident on how to do that) or let subsequent passes deal with that. Ideas? Thanks!

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 39109.Nov 3 2015, 1:22 PM
davide retitled this revision from to [SimplifyLibCalls] Add a new transform: tan(atan(x)) -> x.
davide updated this object.
davide added reviewers: majnemer, scanon, escha, resistor.
davide added a subscriber: llvm-commits.
majnemer added inline comments.Nov 3 2015, 1:25 PM
lib/Transforms/Utils/SimplifyLibCalls.cpp
1382–1385 ↗(On Diff #39109)

No new instructions are created, why is this present?

1392–1394 ↗(On Diff #39109)

The parens around Func == LibFunc::atan don't seem necessary. Also, I'd flip the order of the comparisons so that we are comparing Func first.

test/Transforms/InstCombine/tan.ll
20 ↗(On Diff #39109)

Attribute #0 and #1 are identical, I'd switch all users over to #0.

scanon edited edge metadata.Nov 3 2015, 1:36 PM

I'd like to see this made generic so it applies to any 1-to-1 libm function whose inverse is available, since the same pattern applies for logN(expN(x)), asinh(sinh(x)), atanh(tanh(x)), etc ...

davide updated this revision to Diff 39113.Nov 3 2015, 1:51 PM
davide edited edge metadata.

Addressed David's comments.

davide added a comment.Nov 3 2015, 1:52 PM

I'd like to see this made generic so it applies to any 1-to-1 libm function whose inverse is available, since the same pattern applies for logN(expN(x)), asinh(sinh(x)), atanh(tanh(x)), etc ...

I'll prepare a generalized version of this patch. Thanks.

davide updated this revision to Diff 39115.Nov 3 2015, 1:54 PM

Upload correct version of the patch.

majnemer added inline comments.Nov 3 2015, 2:15 PM
lib/Transforms/Utils/SimplifyLibCalls.cpp
1381 ↗(On Diff #39115)

I think it would be nicer to do an early return:

auto *OpC = dyn_cast<CallInst>(Op1);
if (!OpC)
  return Ret;
1382 ↗(On Diff #39115)

Is the guard needed?

davide updated this revision to Diff 39132.Nov 3 2015, 4:07 PM

Updated after second round of comments. Thank you for your review, David.

davide added a comment.Nov 4 2015, 3:24 PM

Also, forgot to mention. I would like to get this as-is (given it works) and then generalize it as next step, unless David/Stephen have objections.

majnemer accepted this revision.Nov 4 2015, 3:29 PM
majnemer edited edge metadata.

LGTM

test/Transforms/InstCombine/tan.ll
11–14 ↗(On Diff #39132)

You just need a single check statement after the CHECK-LABEL directive, ; CHECK: ret float %x

This revision is now accepted and ready to land.Nov 4 2015, 3:29 PM
This revision was automatically updated to reflect the committed changes.