Page MenuHomePhabricator

[InstCombine] Missed optimization in math expression: aggressive optimization with pow
Needs ReviewPublic

Authored by Quolyk on Dec 27 2017, 11:30 PM.

Details

Summary

Motivation: https://bugs.llvm.org/show_bug.cgi?id=35595. pow(a, x) * a * a * a * a -> pow(a, x+4) is not yet implemented.

Diff Detail

Event Timeline

Quolyk created this revision.Dec 27 2017, 11:30 PM

I'm slightly worried about all this bunch of missing instcombines added, as InstCombine is already really really slow.

That said, this one is probably one we really want. I skimmed the code quickly and I think it's correct, but please wait for somebody else to take a look

Why is this patch WIP? The mentioned case of 'pow(a, x) * a * a * a * a -> pow(a, x+4)' is handled already by this patch.

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
705–708

You mentioned that you see warnings without explicitly setting these to nullptr in Xcode. I use Xcode as an IDE too, but I don't see any warnings like that when I remove the nullptrs.

If this is really a problem, then you must be getting hundreds of these warnings for existing code that does not initialize things like this? I'd prefer not to bloat the code unnecessarily.

713

Need to handle commuted versions too (please add a test):

define double @pow_ab_x_a_fast_commute(double %a, double %b)  {
  %c = fdiv double 1.0, %a  ; defeat complexity-based canonicalization of operands
  %p = call fast double @llvm.pow.f64(double %a, double %b)
  %mul = fmul fast double %c, %p
  ret double %mul
}
714–715

Here and below: use m_Specific instead of the trailing check for equality?

723–724

Please use variable names that match the formulas in the code comments for better readability.

Quolyk updated this revision to Diff 128876.Jan 7 2018, 9:53 AM

pow(a, x) * a * a * a * a emits to

define double @pow_ab_x_aaaa_fast(double %a, double %x) {
  %1 = call fast double @llvm.pow.f64(double %a, double %x)
  %2 = fmul fast double %a, %a
  %3 = fmul fast double %2, %2
  %mul4 = fmul fast double %3, %1
  ret double %mul4
}

I don't see obvious ways to fold these instructions. I Would appreciate if somebody could help me with this.

Quolyk marked 4 inline comments as done.Jan 7 2018, 9:54 AM
Quolyk retitled this revision from [WIP][InstCombine] Missed optimization in math expression: aggressive optimization with pow to [InstCombine] Missed optimization in math expression: aggressive optimization with pow.Jan 14 2018, 11:58 PM
Quolyk added a reviewer: efriedma.

pow(a, x) * a * a * a * a emits to

define double @pow_ab_x_aaaa_fast(double %a, double %x) {
  %1 = call fast double @llvm.pow.f64(double %a, double %x)
  %2 = fmul fast double %a, %a
  %3 = fmul fast double %2, %2
  %mul4 = fmul fast double %3, %1
  ret double %mul4
}

I don't see obvious ways to fold these instructions. I Would appreciate if somebody could help me with this.

This looks like it went through -reassociate first? I was checking with a straight IR translation, so we're just multiplying by 'a' over and over. This would require more complex logic, so that's a different patch (and I'm not sure where it would belong).

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
713

This comment was marked 'Done', but I don't see code to account for this or the test that I suggested.

Quolyk marked an inline comment as not done.Jan 17 2018, 11:01 AM
Quolyk added inline comments.
lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
713

My bad, I thought fdiv_pow_ab_a test would be enough.