Page MenuHomePhabricator

[Target] remove TargetRecip class; move reciprocal estimate isel functionality to TargetLowering
ClosedPublic

Authored by spatel on Oct 10 2016, 10:13 AM.

Details

Summary

This is a follow-up to D24816 - where we changed reciprocal estimates to be function attributes rather than TargetOptions.

This patch is intended to be a structural, but not functional change. By moving all of the TargetRecip functionality into TargetLowering, we can remove all of the reciprocal estimate state, shield the callers from the string format implementation, and simplify/localize the logic needed for a target to enable this.

If a function has a "reciprocal-estimates" attribute, those settings may override the target's default reciprocal preferences for whatever operation and data type we're trying to optimize. If there's no attribute string or specific setting for the op/type pair, just use the target default settings.

As noted earlier, a better solution would be to move the reciprocal estimate settings to IR instructions and SDNodes rather than function attributes, but that's a multi-step job that requires infrastructure improvements. I intend to work on that, but it's not clear how long it will take to get all the pieces in place.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 74141.Oct 10 2016, 10:13 AM
spatel retitled this revision from to [Target] remove TargetRecip class; move reciprocal estimate isel functionality to TargetLowering .
spatel updated this object.
spatel added a subscriber: llvm-commits.
hfinkel edited edge metadata.Oct 17 2016, 11:40 AM

I think this is an improvement. Can we go a bit further in order to further remove the boilerplate here and move the calls to getSqrtEnabled and, if not disabled, getSqrtRefinementSteps into the caller of getRsqrtEstimate (along with the analogous change for the Div estimates)? All of the implementations use 'VT = Operand.getValueType()'.

spatel updated this revision to Diff 75014.Oct 18 2016, 8:48 AM
spatel edited edge metadata.

Patch updated:
Implement Hal's suggestion to reduce the repeated code by pulling getRsqrtEstimate/getSqrtRefinementSteps and the Div siblings into DAGCombiner. This changes the hook interface, so the patch grows a bit, but this is less ugly overall. :)

spatel updated this revision to Diff 75017.Oct 18 2016, 9:02 AM
spatel edited edge metadata.

Patch updated:
There was a copy/paste bug in the DAGCombiner hunk - it was checking the function override for div rather than sqrt. We don't have very good test coverage for checking those function attribute strings.

hfinkel accepted this revision.Oct 18 2016, 9:04 AM
hfinkel edited edge metadata.

Patch updated:
Implement Hal's suggestion to reduce the repeated code by pulling getRsqrtEstimate/getSqrtRefinementSteps and the Div siblings into DAGCombiner. This changes the hook interface, so the patch grows a bit, but this is less ugly overall. :)

Thanks, this LGTM.

This revision is now accepted and ready to land.Oct 18 2016, 9:04 AM
This revision was automatically updated to reflect the committed changes.
echristo edited edge metadata.Oct 18 2016, 10:45 AM

I had one inline bikeshed type of comment, but otherwise also LGTM. Thanks for doing this work!

-eric

include/llvm/Target/TargetLowering.h
260–266 ↗(On Diff #75017)

Bit of a bikeshed but these two functions sound like they're seeing if sqrt and div are instructions in the hardware.

spatel added inline comments.Oct 18 2016, 1:45 PM
include/llvm/Target/TargetLowering.h
260–266 ↗(On Diff #75017)

I'll take any suggested improvements. :)

Note that I reverted this commit at rL284513 because I screwed something up with the StringRefs while parsing the function attribute.

A bot said: "AddressSanitizer: initialization-order-fiasco" (!)