[PATCH] D51435: [SLC] Support expanding pow(x, n+0.5) to x * x * ... * sqrt(x)

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for minor improvements.



================
Comment at: lib/Transforms/Utils/SimplifyLibCalls.cpp:1400
   if (Pow->isFast() && match(Expo, m_APFloat(ExpoF))) {
     // We limit to a max of 7 multiplications, thus the maximum exponent is 32.
     APFloat LimF(ExpoF->getSemantics(), 33.0),
----------------
fhahn wrote:
> spatel wrote:
> > There was no real justification for this limit in D13994, but I suppose we're still ok with the transform +1 more instruction?
> IIUC the main reason for adding this limit was to avoid generating too long fmul chains. I think adding a call to sqrt() is independent of that (similar to adding a call to fdiv for negative exponents), but I can either update the comment or only generate sqrt() for smaller exponents.
The sqrt enhancement was mentioned in the original patch, so I won't hold this patch up...
But this entire transform is questionable as an IR canonicalization (instcombine). The limit was chosen arbitrarily, and it applies universally even though the optimal limit will vary based on target. We're also doing this transform regardless of whether we are optimizing for size or not.

There was a suggestion in the original patch that this should be a backend transform, and I think that was correct. Alternatively, there should be a backend reversal of this transform if the target would prefer to use a libcall instead of expanding. 

This should be noted in a TODO comment here.


================
Comment at: test/Transforms/InstCombine/pow-4.ll:137
+; pow(x, -16.5)
+define double @test_simplify_neg_16_5(double %x) {
+; CHECK-LABEL: @test_simplify_neg_16_5(
----------------
It would be nice to get a bit more coverage from these tests by varying the exponent and data types (the transform should work with vectors too?).


https://reviews.llvm.org/D51435