[Mesa-dev] [PATCH 20/24] Revert "i965: Have brw_imm_vf4() take the vector components as integer values."

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> On Tue, Nov 3, 2015 at 5:16 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Matt Turner <mattst88 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151104/5aaabc54/attachment.sig>