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

[Kenneth Graunke]

On Friday, October 24, 2014 08:47:03 PM Francisco Jerez 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)));

It might not be a bad idea to have both.  I agree with Curro - being able to 
say src_reg(0, 8, 16, 24) is nice and easy to read.  But being able to pass in 
pre-converted values is not a bad idea either, and makes Matt's case work.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141024/094c5ba5/attachment-0001.sig>