[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 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?

Not necessarily, we don't really need the arguments to be converted at
compile time if we use a dynamic assert.
-------------- 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/754dc5f9/attachment-0001.sig>