Page MenuHomePhabricator

[InstSimplify] Missed optimization in math expression: squashing exp(log), log(exp)
ClosedPublic

Authored by Quolyk on Dec 19 2017, 2:21 AM.

Details

Summary

This patch enables folding following expressions under -ffast-math flag: exp(log(x)) -> x, exp2(log2(x)) -> x, log(exp(x)) -> x, log2(exp2(x)) -> x

Diff Detail

Event Timeline

Quolyk created this revision.Dec 19 2017, 2:21 AM
hfinkel accepted this revision.Dec 19 2017, 7:53 PM

LGTM

This revision is now accepted and ready to land.Dec 19 2017, 7:53 PM

I don't have commit access, please commit.

davide requested changes to this revision.Dec 20 2017, 1:32 AM
davide added a subscriber: scanon.

I don't have access to my checkout here, but I'm fairly sure we do something similar in SimplifyLibCalls.
I understand many of the pattern proposed will be duplicated, which is, not ideal.
Also, I'm pretty sure @scanon once taught me (when I implemented a similar transformation in simplifylibcalls) that these can underflow/overflow pretty dramatically, so, shouldn't these only be enabled under -ffast-math ?

This revision now requires changes to proceed.Dec 20 2017, 1:32 AM
Quolyk added a comment.EditedDec 20 2017, 11:10 PM

I don't have access to my checkout here, but I'm fairly sure we do something similar in SimplifyLibCalls.
I understand many of the pattern proposed will be duplicated, which is, not ideal.
Also, I'm pretty sure @scanon once taught me (when I implemented a similar transformation in simplifylibcalls) that these can underflow/overflow pretty dramatically, so, shouldn't these only be enabled under -ffast-math ?

These transforms are only enabled under -ffast-math, II->isFast() checks here for it. Tests explicitly show correctness. SimlifyLibCalls doesn't have these optimizations, but I can move transforms to that file if it's necessary.

Quolyk requested review of this revision.Dec 20 2017, 11:18 PM
Quolyk updated this revision to Diff 127835.Dec 21 2017, 12:26 AM
davide accepted this revision.Dec 23 2017, 5:31 AM

Sorry, I clearly misread your previous diff. LGTM.

This revision is now accepted and ready to land.Dec 23 2017, 5:31 AM

Since this hasn't been committed yet, let me ask: why are we doing this in instcombine rather than instsimplify? When no new instructions are created, we generally want that transform in InstSimplify because that makes passes like GVN more powerful. InstSimplify already has "SimplifyIntrinsic", so I think this could fit neatly in there.

Since this hasn't been committed yet, let me ask: why are we doing this in instcombine rather than instsimplify? When no new instructions are created, we generally want that transform in InstSimplify because that makes passes like GVN more powerful. InstSimplify already has "SimplifyIntrinsic", so I think this could fit neatly in there.

Indeed, my oversight. Yes, it would be good to have this in InstSimplify.

Quolyk updated this revision to Diff 128406.EditedJan 2 2018, 4:05 AM
Quolyk retitled this revision from [InstCombine] Missed optimization in math expression: squashing exp(log), log(exp) to [InstSimplify] Missed optimization in math expression: squashing exp(log), log(exp).
Quolyk edited the summary of this revision. (Show Details)

Moved to InstructionSimplify. This patch changed a bit, ping me if ok, I'll commit.

spatel added a comment.Jan 2 2018, 8:08 AM

What is the expected result for this kind of test (please add at least one test like this)?

define double @exp_log_partly_fast(double %a) {
  %1 = call double @llvm.log.f64(double %a)          ; strict
  %2 = call fast double @llvm.exp.f64(double %1)
  ret double %2
}
Quolyk updated this revision to Diff 128440.Jan 2 2018, 11:47 AM

Updated tests

spatel accepted this revision.Jan 2 2018, 2:28 PM

LGTM - see inline comments for some potential improvements.

lib/Analysis/InstructionSimplify.cpp
4520

I don't see much value in naming this local (could just use *ArgBegin in the line below here).

So you could shrink this down to:

Value *X;
if (Q.CxtI->isFast() && match(*ArgBegin, m_Intrinsic<Intrinsic::log>(m_Value(X))))
  return X;
return nullptr;
4521

As I've said in other reviews, I think explicitly setting this to nullptr is just noise.

test/Transforms/InstSimplify/exp-intrinsic.ll
5–6

It's a matter of taste, but since these are highly similar transforms, I'd put all of the tests together in one test file to match the fact that they're all handled together in one block of code.

28–46

Looking back at the discussion in D5584, I think this is correct. Only the later op needs to be relaxed to do the transform.

Quolyk updated this revision to Diff 128493.Jan 3 2018, 12:09 AM

@spatel I updated diff, please take a look.

spatel accepted this revision.Jan 3 2018, 6:26 AM

LGTM - thanks for the clean-up.

Quolyk closed this revision.Jan 3 2018, 6:38 AM