There already exists an optimization which try to convert pow(X, 0.25) to sqrt(sqrt(x)). This patch extends it with pow(X, 0.75).
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Please, add a test for AArch64 and new cases in test/Transforms/InstCombine/pow-sqrt.ll as well.
@evandro Why do we want to add new cases in test/Transforms/InstCombine/pow-sqrt.ll? Am I right that InstCombine tests opt? This new changes happen in llc.
We have similar implications in lib/Transforms/Utils/SimplifyLibCalls.cpp. This does not really seem DAG specific. Maybe it would be better suited for SimplifyLibCalls.cpp?
@fhahn We have also consider adding to SimplifyLibCalls.cpp. Since pow(X, 0.25) was proposed as an IR transform in D49306, but it was not clearly justifiable as a canonicalization, and pow(X, 0.25) is implemented in DAGCombiner, we decided that it would be best to put pow(X, 0.75) in DAGCombiner as well.
llvm/test/CodeGen/X86/pow.75.ll | ||
---|---|---|
2 ↗ | (On Diff #184364) | These will all break without REQUIRES: asserts |
After the nsz check is addressed, it LGTM.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
11852 ↗ | (On Diff #184364) | This. |
llvm/test/CodeGen/X86/pow.75.ll | ||
---|---|---|
2 ↗ | (On Diff #184364) | What I am trying to check is that pow(x, 0.75) gets transformed into sqrt(x)*sqrt(sqrt(x)). I wanted to check at an earlier stage rather than asm, because asm requests the reader to known signifiant details about the expansions to know that this is the right thing or not, which makes for a very confusing and hard to read test case. |
Whitney, if you don't have commit access let me know and I can commit this for you. If so, I'll fix the REQUIRES: asserts in the test cases and fix the no-signed-zeros requirement on the commit.