Page MenuHomePhabricator

libclc: Add a __builtin to let SPIRV targets select between SW and HW FMA
ClosedPublic

Authored by daniels on Aug 13 2020, 8:27 AM.

Details

Summary

The SPIRV targets are supposed to be generic, but some HW still don't
support the Fma instruction. Add the __builtin_has_hw_fma32() builtin
function to let the implementation express whether it has hardware FMA
support or not.

Depends on: D85911

Diff Detail

Event Timeline

daniels created this revision.Aug 13 2020, 8:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2020, 8:27 AM
daniels requested review of this revision.Aug 13 2020, 8:27 AM
jenatali requested changes to this revision.Aug 22 2020, 9:01 AM
jenatali added a subscriber: jenatali.
jenatali added inline comments.
libclc/generic/lib/math/math.h
44

From discussion on the corresponding Mesa merge request that would handle this builtin function call (https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6035#note_606348), I think this would be better as defined, non-inlined function. That way, we get the best of both worlds:

  • If Mesa understands the builtin, it can choose to override it with its own implementation, rather than using the one provided by libclc, allowing individual drivers to control the behavior.
  • If mesa doesn't understand the builtin, then it's just a regular function call, and after non-LLVM optimizations, it will eventually end up inlined, and the dead code for the non-taken path can be removed.

We might need to have a dedicated .cl file for the definition though, rather than defining it inline, just to avoid violating the one-definition-rule here. And we should probably do the same thing for subnormal controls, rather than just setting them to false.

This revision now requires changes to proceed.Aug 22 2020, 9:01 AM
daniels updated this revision to Diff 287980.Aug 26 2020, 8:11 AM

Added noinline to use software by default, letting implementation override

daniels marked an inline comment as done.Aug 26 2020, 8:11 AM
daniels edited the summary of this revision. (Show Details)
jenatali accepted this revision.Aug 26 2020, 8:14 AM

Yep, I think this is exactly what we need.

This revision is now accepted and ready to land.Aug 26 2020, 8:14 AM
jvesely added inline comments.Aug 31 2020, 8:13 AM
libclc/generic/lib/math/math.h
45

This shouldn't be prefixed __bultin that space is reserved for the compiler. Naming it something like __clc_runtime_has_fma32 would be more fitting.

daniels added inline comments.Aug 31 2020, 8:15 AM
libclc/generic/lib/math/math.h
45

Fine by me. @jenatali?

jenatali added inline comments.Aug 31 2020, 8:56 AM
libclc/generic/lib/math/math.h
45

No complaints.

jvesely requested changes to this revision.Aug 31 2020, 9:05 AM
This revision now requires changes to proceed.Aug 31 2020, 9:05 AM
daniels updated this revision to Diff 291232.Sep 11 2020, 8:40 AM

Change builtin to clc_runtime

daniels marked 2 inline comments as done.Sep 11 2020, 8:40 AM

Done, @jvesely could you please merge this one for us too?

jvesely accepted this revision.Sep 15 2020, 9:55 AM
This revision is now accepted and ready to land.Sep 15 2020, 9:55 AM