This is an archive of the discontinued LLVM Phabricator instance.

Optimize pow(X, 0.75) to sqrt(X) * sqrt(sqrt(X))
ClosedPublic

Authored by Whitney on Jan 29 2019, 6:39 PM.

Details

Summary

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).

Diff Detail

Repository
rL LLVM

Event Timeline

Whitney created this revision.Jan 29 2019, 6:39 PM

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.

Whitney updated this revision to Diff 184364.Jan 30 2019, 1:55 PM

@evandro Added a test for AArch64.

@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.

Right, thanks! I wasn't aware of the discussion there.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 10:25 AM

LG in principle.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11852 ↗(On Diff #184364)

Looks like this doesn't need to be guarded by nsz flag?

llvm/test/CodeGen/X86/pow.75.ll
2 ↗(On Diff #184364)

Why -debug, instead of checking the asm?

arsenm added a subscriber: arsenm.Feb 6 2019, 2:51 PM
arsenm added inline comments.
llvm/test/CodeGen/X86/pow.75.ll
2 ↗(On Diff #184364)

These will all break without REQUIRES: asserts

@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.

Ah, OK. It makes sense.

evandro accepted this revision.Feb 7 2019, 8:39 AM

After the nsz check is addressed, it LGTM.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11852 ↗(On Diff #184364)

This.

This revision is now accepted and ready to land.Feb 7 2019, 8:39 AM
Whitney added inline comments.Feb 7 2019, 5:15 PM
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.

@nemanjai That will be great, thanks!

This revision was automatically updated to reflect the committed changes.