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

[Matt Turner]

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.

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.

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.

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.

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