[PATCH] D49306: [SLC] Simplify pow(x, 0.25) to sqrt(sqrt(x))

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D49306#1183905, @lebedev.ri wrote:

> And the same remarks are probably applicable to https://reviews.llvm.org/D49040 and https://reviews.llvm.org/D50036 too, even though they only
>  extend the existing code. Canonicalizing all the `sqrt`/`cbrt`/... to just one `pow` will
>  make it simpler for everything else since it's just one function to be aware of, instead of three.


It's fuzzy. If we had decided on pow() as the canonical form for sqrt() from the start, that would be true. But now we have an intrinsic, analysis, transforms, cost modeling, lowering, etc. for sqrt() directly. So if we wanted to reverse that, we'd have to modify all of that to recognize pow(x, 0.5) as the equivalent. I think it's better as we have it now because sqrt() is the more recognized form both in source code and hardware.

IMO, what made this patch different than the others is that we're replacing 1 call with 2 calls (and potentially other ops as shown in the tests here). Again though, it's fuzzy. We could argue that because sqrt() has existing analysis and pow() doesn't, the longer sequence in IR is better. That isn't the usual way to canonicalize, but it's not unprecedented. For example, there are existing transforms where we turn 1 sdiv/udiv into logic+cmp+ select.

I would lean towards converting cbrt() to pow() here in IR. AFAIK, there's no existing IR benefit to the cbrt() form. Plus, there is an intrinsic for pow(), so that makes vectorization easier. If we canonicalize in the other direction (pow(x, 0.33) --> cbrt(x)), then we have a canonicalization difference based on scalar/vector...or we have to add an intrinsic for cbrt?


https://reviews.llvm.org/D49306