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

[Roland Scheidegger]

Am 13.10.2015 um 00:26 schrieb Rob Clark:
> On Mon, Oct 12, 2015 at 6:07 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> 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).
> 
> the use from pack/unpack_half_1x16 in nir/ir_constant_expression made
> me assume it is more about non-render-target format stuff and more
> about constant propagation optimizations in the shader.. but I don't
> know quite where this is used or the full history of it..
> 
The gallium version will at least be used for converting clear colors to
f16 for such render targets for sw based drivers too. And because GL
doesn't care but d3d10 does that's really why it rounds too large values
down (I was also under the impression since exactly due to GL not caring
but d3d10 requiring a specific behavior hw would just follow d3d10 rules
there but I might have been wrong...).
And while GL doesn't seem to care for such conversions, opencl just uses
default rounding mode (so generally round to nearest even) too for such
conversions (unless otherwise specified), so I guess d3d is a bit of an
outlier there by requiring round-to-zero for (float) conversions.

Roland