[Mesa-dev] [PATCH 03/10] i965/fs: Add vector float immediate infrastructure.

[Matt Turner]

On Fri, Oct 24, 2014 at 11:12 AM, Matt Turner <mattst88 at gmail.com> wrote:
> On Fri, Oct 24, 2014 at 10:47 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Matt Turner <mattst88 at gmail.com> writes:
>>
>>> On Fri, Oct 24, 2014 at 1:34 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>>> Matt Turner <mattst88 at gmail.com> writes:
>>>>
>>>>> ---
>>>>>  src/mesa/drivers/dri/i965/brw_fs.cpp           | 20 ++++++++++++++++++++
>>>>>  src/mesa/drivers/dri/i965/brw_fs.h             |  1 +
>>>>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp |  3 +++
>>>>>  3 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>>>> index 983e8db..ffe0a5b 100644
>>>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>>>> @@ -44,6 +44,7 @@ extern "C" {
>>>>>  #include "brw_context.h"
>>>>>  #include "brw_eu.h"
>>>>>  #include "brw_wm.h"
>>>>> +#include "brw_packed_float.h"
>>>>>  }
>>>>>  #include "brw_fs.h"
>>>>>  #include "brw_cfg.h"
>>>>> @@ -581,6 +582,18 @@ fs_reg::fs_reg(uint32_t u)
>>>>>     this->width = 1;
>>>>>  }
>>>>>
>>>>> +/** Vector float immediate value constructor. */
>>>>> +fs_reg::fs_reg(uint8_t vf[4])
>>>>> +{
>>>>> +   init();
>>>>> +   this->file = IMM;
>>>>> +   this->type = BRW_REGISTER_TYPE_VF;
>>>>> +   this->fixed_hw_reg.dw1.ud = (vf[0] <<  0) |
>>>>> +                               (vf[1] <<  8) |
>>>>> +                               (vf[2] << 16) |
>>>>> +                               (vf[3] << 24);
>>>>> +}
>>>>> +
>>>>
>>>> IMHO it would be a lot more convenient to have this function take four
>>>> float arguments and carry out the conversion to 8-bit here.  I suspect
>>>> that in most cases the arguments of this constructor (and the same goes
>>>> for src_reg) will be known at compile time so we could just add an
>>>> assertion to check that the conversion was carried out exactly.
>>>
>>> That would work if we're constructing VF immediates from known good
>>> values, but in cases like in an optimization, you're always going to
>>> have to check whether the values are representable in VF. If they're
>>> not, the pass will have to fail.
>>>
>>
>> Yeah, but isn't this interface unnecessarily annoying to use in the
>> cases that fit the "known good values" category? (all uses in your
>> series and in my previous code that makes use of VF) -- It led you write
>> code like this:
>>
>> | +   uint8_t vf[4] = {0x0, 0x60, 0x70, 0x78};
>> |[...]
>> | +   emit(MOV(shift, src_reg(vf)));
>>
>> ...which is pretty hard to decode.  Don't you find this much easier to
>> understand? (even if it means we may have to introduce a second
>> constructor at some point in the future)
>>
>> |   emit(MOV(shift, src_reg(0, 8, 16, 24)));

Another question, if I do this would you have me make
brw_float_to_vf() a static inline function so that the arguments can
be converted at compile time?