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

[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..

BR,
-R

> 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
>