Page MenuHomePhabricator

[DAGCombiner] try to convert pow(x, 1/3) to cbrt(x)

Authored by spatel on Sep 6 2018, 2:16 PM.



This is a follow-up suggested in D51630 and originally proposed as an IR transform in D49040.

Copying the motivational statement by @evandro from that patch:
"This transformation helps some benchmarks in SPEC CPU2000 and CPU2006, such as 188.ammp, 447.dealII, 453.povray, and especially 300.twolf, as well as some proprietary benchmarks. Otherwise, no regressions on x86-64 or A64."

I'm proposing to add only the minimum support for a DAG node here. Since we don't have an LLVM IR intrinsic for cbrt, and there are no other DAG ways to create a FCBRT node yet, I don't think we need to worry about DAG builder, legalization, a strict variant, etc. We should be able to expand as needed when adding more functionality/transforms. For reference, these are transform suggestions currently listed in SimplifyLibCalls.cpp:

//   * cbrt(expN(X))  -> expN(x/3)
//   * cbrt(sqrt(x))  -> pow(x,1/6)
//   * cbrt(cbrt(x))  -> pow(x,1/9)

Also, given that we bail out on long double for now, I don't think there will be any logical differences between platforms, but let me know if you'd like to see test coverage for other targets.

Diff Detail


Event Timeline

spatel created this revision.Sep 6 2018, 2:16 PM
lebedev.ri added inline comments.Sep 7 2018, 12:11 AM
2 ↗(On Diff #164283)

There is _mm_pow_ps() and _mm_cbrt_ps() in SSE.
Maybe there should be an -mattr=+sse runline?

spatel added inline comments.Sep 7 2018, 6:25 AM
2 ↗(On Diff #164283)

I didn't know about those. They're SVML library calls, right? Or is there an x86 hardware target somewhere out there?

There is support for the pow SVML call in LLVM, but I don't see cbrt anywhere. Either way, I'm not sure how that's testable here because SVML transforms happen in IR currently ( related discussion in: )

mattr=+sse is a requirement of x86-64, so how does making that explicit change things?

lebedev.ri accepted this revision.Sep 12 2018, 8:49 AM

Looks good, but would be good for someone else to take a look, too.

181–183 ↗(On Diff #164283)

Will strict_fcbrt be needed, too?

This revision is now accepted and ready to land.Sep 12 2018, 8:49 AM

Looks good, but would be good for someone else to take a look, too.

Thanks! I'll wait a few days at least for more feedback.

181–183 ↗(On Diff #164283)

Not sure. IIUC, we would want that if there's a possibility of converting *from* cbrt to something else. Here, we're only transforming *to* cbrt, so I don't see how a strict sibling would come into play at this point.

This revision was automatically updated to reflect the committed changes.