[Mesa-dev] [PATCH 3/6] glsl: move half<->float convertion to util

[Roland Scheidegger]

Am 12.10.2015 um 22:37 schrieb Rob Clark:
> On Mon, Oct 12, 2015 at 3:41 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 12.10.2015 um 20:33 schrieb Rob Clark:
>>> On Mon, Oct 12, 2015 at 2:22 PM, Matt Turner <mattst88 at gmail.com> wrote:
>>>> On Mon, Oct 12, 2015 at 11:12 AM, Rob Clark <robdclark at gmail.com> wrote:
>>>>> On Mon, Oct 12, 2015 at 12:47 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>>>>> +/**
>>>>>>>> + * Convert a 2-byte half float to a 4-byte float.
>>>>>>>> + * Based on code from:
>>>>>>>> + * https://urldefense.proofpoint.com/v2/url?u=http-3A__www.opengl.org_discussion-5Fboards_ubb_Forum3_HTML_008786.html&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=Vjtt0vs_iqoI31UfJxBl7yv9I2FeiaeAYgMTLKRBc_I&m=2SV7p2GaUU6_-p4UXWRZ9hppJQ1vlbvvFipu3TmlI7s&s=B2vMgNT8F0lMSh_aJLIvB2ErGZNaLSKmlOgEdZq7z9Y&e= 
>>>>>>>> + */
>>>>>>>> +static inline float
>>>>>>>> +_mesa_half_to_float(GLhalfARB val)
>>>>>>>> +{
>>>>>>>> +   return half_to_float(val);
>>>>>>>> +}
>>>>>>
>>>>>> This is kind of ugly. How hard would it really be to just replace the uses
>>>>>> with the new name?  I don't think its used *that* often.
>>>>>
>>>>> hmm, ~20-30 call sites.. not impossible to update them all, but I
>>>>> think it should be a separate patch and then drop the compat shims
>>>>> rather than one big patch that changes everything..
>>>>>
>>>>> We could also keep the _mesa_half_to_float() name.. but I really
>>>>> wanted to drop the GLhalfARB and not drag along GL typedefs.  If no
>>>>> one objects to just replacing GLhalfARB with uint16_t.
>>>>>
>>>>> I could go either way.
>>>>
>>>> Replacing GLhalfARB with uint16_t seems like a good plan.
>>>
>>> ok, then the most straightforward (least churn) approach seems to be
>>> to keep the _mesa_ prefix but use uint16_t..
>>>
>>>>>>>> +
>>>>>>>> +#include <stdint.h>
>>>>>>>> +
>>>>>>>> +#ifdef __cplusplus
>>>>>>>> +extern "C" {
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +uint16_t float_to_half(float val);
>>>>>>>> +float half_to_float(uint16_t val);
>>>>>>>
>>>>>>> I think these functions need to be prefixed with something -- util_*
>>>>>>> or something or just leave them as _mesa_*.
>>>>>>
>>>>>> Unfortunately, until is kind of a grab-bag right now.  Some stuff has a
>>>>>> util_ prefix, some has kept its _mesa_ or u_ prefix depending on whether it
>>>>>> was copied from Mesa or gallium, and some (hash_table for example) isn't
>>>>>> prefixed at all.  Personally, I'd go with either util_ (it is a utility
>>>>>> library) or just keep _mesa_ (this is still the mesa project after all). I'm
>>>>>> not going to be too insistent though
>>>>>
>>>>> the util_ prefix conflicts w/ u_half.h (which appears to implement
>>>>> basically the same thing in a simpler way, but maybe not compatible
>>>>> enough to just switch to the gallium implementation?  Otherwise the
>>>>> easier approach would be to just move the gallium implementation to
>>>>> global util directory and use that instead)..  I was going to use
>>>>> convert_ prefix but that also conflicts..
>>>>
>>>> Seems valuable to ultimately remove the Gallium implementation as
>>>> well. Chad did some really tedious work to improve the functions
>>>> you're moving to round properly.
>>>
>>> hmm, looks like his change was mostly important for compiler (to match
>>> what intel hw does).. otoh I guess it shouldn't hurt for gallum (which
>>> looks to be used mostly for texture/format conversion) and would be
>>> nice to de-duplicate..
>>
>> I'd be in favor of dropping this implementation over the gallium one,
>> but not vice versa. I've expressed my concerns over this implementation
>> before (it also looks more expensive), mainly rounding up float values
>> to infinity half floats seems like a bad idea. The gallium
>> implementation has a comment why I think is a bug, mostly because d3d10
>> mandates such value getting rounded to MAX_HALF_FLOAT instead, gl not
>> caring about it but at least recommending the same for converting to
>> fp11 floats. I doubt hw in general rounds such values to infinity (due
>> to the afromentioned d3d10 requirement, which is actually tested for).
> 
> well, I'll leave the gallium version as-is then..  I guess I can
> understand where (for the using in compiler const propagation) there
> is a desire to match what intel hw does (but otoh not really sure if
> it matches what other hw does and if gl doesn't really care, not sure
> how much that really matters).  But at least for now to clean up the
> NIR dependency on GLSL I guess we should keep the current _mesa
> version..

As far as I can tell (and I could easily be wrong here...) intel hw
doesn't have explicit conversion instruction, you just say it's a f16
destination. As such I believe the conversion used would just use
ordinary rounding mode, which is of course usually round to nearest even
(and the docs don't seem to mention it, but seems likely that too large
floats would get converted to infinities using that mode). Rounding mode
is of course changeable (but would need to change it back after f32->f16
mov).
On (gcn) radeons, I suspect it's the same, though there's different
instruction encodings which can take a arbitrary rounding mode, thus you
could just use that (albeit that encoding needs more bits).
(Even on intel cpus supporting half-float conversion instructions it
works that way, though you can just set a bit so the rounding mode comes
from the immediate byte, which is what llvmpipe uses, to set it to
truncate.)

Since d3d10 doesn't really support half-floats, there might be hw which
can't do it as ordinary instruction (?) - in such a case I would expect
this to be part of shader export or ROP (as such hw still would need to
be able to write to fp16 fbs), which would very likely just follow d3d10
rules.

Roland