This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Ensure trig range reduction only used for subtargets that require it
ClosedPublic

Authored by dstuttard on Sep 11 2018, 7:52 AM.

Details

Summary

GFX9 and above support sin/cos instructions with a greater range and thus don't
require a fract instruction prior to invocation.

Added a subtarget feature to reflect this and added code to take advantage of
expanded range on GFX9+

Also updated the tests to check correct behaviour

Diff Detail

Repository
rL LLVM

Event Timeline

dstuttard created this revision.Sep 11 2018, 7:52 AM
nhaehnle accepted this revision.Sep 12 2018, 3:42 AM

One nitpick, apart from that LGTM.

lib/Target/AMDGPU/SIISelLowering.cpp
6653–6661 ↗(On Diff #164880)

Factor out the FMUL part so that it's more obvious that the only difference is in the FRACT?

This revision is now accepted and ready to land.Sep 12 2018, 3:42 AM
dstuttard updated this revision to Diff 165105.Sep 12 2018, 9:48 AM

Made suggested change

dstuttard marked an inline comment as done.Sep 12 2018, 9:50 AM
arsenm added inline comments.Sep 12 2018, 8:57 PM
lib/Target/AMDGPU/SIISelLowering.cpp
6655 ↗(On Diff #165105)

Avoid repeating the same constant.

Also we should stop relying on system headers for these constants

dstuttard updated this revision to Diff 165214.Sep 13 2018, 1:17 AM

De-duplicated 1/2PI constant

dstuttard marked an inline comment as done.Sep 13 2018, 1:19 AM
dstuttard added inline comments.
lib/Target/AMDGPU/SIISelLowering.cpp
6655 ↗(On Diff #165105)

Re system headers - why, are you concerned that M_PI may differ between compilers?
Can we defer removing reliance on system headers to a separate change that does this globally?

arsenm added inline comments.Sep 13 2018, 1:59 AM
lib/Target/AMDGPU/SIISelLowering.cpp
6655 ↗(On Diff #165105)

Yes. AFAIK the standard doesn't actually specify the exact rounded value. For other system provided constants, LLVM usually duplicates the definitions and puts them in Support

dstuttard marked an inline comment as done.Sep 13 2018, 4:10 AM
dstuttard added inline comments.
lib/Target/AMDGPU/SIISelLowering.cpp
6655 ↗(On Diff #165105)

That's a reasonable point.

However, would you mind if I defer that to a different change as making changes in Support will spill this change over into a core change - the old code used M_PI before, so it's no worse as it stands.

arsenm added inline comments.Sep 13 2018, 4:28 AM
lib/Target/AMDGPU/SIISelLowering.cpp
6655 ↗(On Diff #165105)

Yes. It also doesn't need to be in support. We already locally defined in for MSVC

This revision was automatically updated to reflect the committed changes.