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

[Francisco Jerez]

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)));


> In that case, you've already done the conversion. See this pass, for example:
>
> http://cgit.freedesktop.org/~mattst88/mesa/commit/?h=line&id=dd9504e28037aee5ade38bb8b8d51bd39a444e12
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141024/fd45cafe/attachment.sig>