On Mon, Oct 12, 2015 at 6:07 PM, Roland Scheidegger <srol...@vmware.com> wrote: > Am 12.10.2015 um 22:37 schrieb Rob Clark: >> On Mon, Oct 12, 2015 at 3:41 PM, Roland Scheidegger <srol...@vmware.com> >> wrote: >>> Am 12.10.2015 um 20:33 schrieb Rob Clark: >>>> On Mon, Oct 12, 2015 at 2:22 PM, Matt Turner <matts...@gmail.com> wrote: >>>>> On Mon, Oct 12, 2015 at 11:12 AM, Rob Clark <robdcl...@gmail.com> wrote: >>>>>> On Mon, Oct 12, 2015 at 12:47 AM, Jason Ekstrand <ja...@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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
