Matt Turner <matts...@gmail.com> writes: > On Tue, Nov 3, 2015 at 5:16 AM, Francisco Jerez <curroje...@riseup.net> wrote: >> Matt Turner <matts...@gmail.com> writes: >> >>> This reverts commit bbf8239f92ecd79431dfa41402e1c85318e7267f. >>> >>> I didn't like that commit to begin with -- computing things at compile >>> time is fine -- but for purposes of verifying that the resulting values >>> are correct, looking up 0x00 and 0x30 in a table is a lot better than >>> evaluating a recursive function. >>> >> FYI the function only ever recurses once in order to handle the sign bit >> orthogonally from the exponent and mantissa. >> >>> Anyway, by making brw_imm_vf4() take the actual 8-bit restricted floats >>> directly (instead of only integral values that would be converted to >>> restricted float), we can use this function as a replacement for the >>> vector float src_reg/fs_reg constructors. >> >> Seems awful to me, it replaces a formula that can be verified correct by >> reading the first paragraph of the description of the restricted float >> format (Gen Graphics » BSpec » 3D-Media-GPGPU Engine » EU Overview » ISA >> Introduction » EU Data Types » Numeric Data Types) with a table of magic >> constants that don't give the slightest insight into the structure of >> the restricted float format and therefore have to be verified >> individually. > > I don't think integer-only conversion functions are useful. The float > <-> vf functions that I added subsume int_to_float8. > That's a fair point, in that case we should switch all users (actually just brw_clip_util) of int_to_float8() to use brw_float_to_vf() instead, if it has really subsumed int_to_float8().
> But that's going towards a repeat of that discussion -- with you > saying that we should have vf constructors that take a float or an int > and does the conversion to restricted float for you, and me saying > having to know a priori whether the inputs can be represented as > restricted float makes that untenable. > I don't care enough to continue that line of discussion, but ISTR I mentioned a few alternatives which I don't think are untenable. > Since the process of checking whether the input is representable is > basically identical to doing the actual conversion, if there has to > be a function that tells you if a value is representable, and it might > as well return the actual restricted float if it is. > > > If your concern is the hardcoded constants for VF_ZERO/ONE, I can get > rid of them and change the code in brw_clip_util.c to > > brw_MOV(p, t_nopersp, brw_imm_vf4(brw_float_to_vf(1.0), > brw_float_to_vf(0.0), > brw_float_to_vf(0.0), > brw_float_to_vf(0.0))); > > (with appropriate indentation, in case gmail messes this up) > > That of course should all compile away into code that is identical to > what we have now. > That seems much better than moving back to hardcoded defines, but I doubt the brw_float_to_vf() calls will compile away unless you have LTO enabled. That's the reason why int_to_float8() was defined as static inline instead of inside a C file. Unfortunately brw_float_to_vf() relies on unspecified behaviour (while doing the type-punning trick), so I'm not convinced that it will be fully evaluated at compile-time on all compilers and platforms, and where it does I'm not convinced that it will necessarily give the same result as when evaluated at run-time. Using logb/ldexp from the C standard library as I believe I suggested when you sent the patch implementing brw_float_to_vf() for review would have been simpler, more portable and can easily be evaluated at compile time. > I'd be happy to change other instances of hard-coded restricted floats > to do this as well. > >> It also makes it impossible to use brw_imm_vf4() with >> dynamically-specified integer constants which was the original >> motivation of this change. > > Unless "integer constants" is really the stressed part of this > sentence, I don't think it's true given the suggestion I just made. > Oops, that was a typo, I meant to say "integer values". >> If the obfuscation is meant as an optimization, I don't think it helps. >> GCC compiles the four int_to_float8() calls from brw_imm_vf4() into a >> single instruction already when the arguments are constant expressions: >> >> 4a9dc9: 48 b8 00 00 00 00 30 movabs $0x3000000000,%rax >> >> (assembly taken from brw_clip_interp_vertex()) > > No, it's not meant as an optimization -- I understand that > int_to_float8(const) compiles away. The main purpose is to make > brw_imm_vf4() usable in the following commits.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev