[Mesa-dev] [PATCH 02/19] i965: Introduce a new brw_inst API.

[Kenneth Graunke]

On Friday, June 13, 2014 11:14:03 PM Matt Turner wrote:
> From: Kenneth Graunke <kenneth at whitecape.org>
> 
> This is similar to gen8_instruction, and will replace it
> 
> For now nothing uses this, but we can incrementally convert.
> The new API takes the existing brw_instruction pointers to ease
> conversion; when done, we can simply drop the old structure and rename
> struct brw_instruction -> brw_inst.
> 
> Reviewed-by: Matt Turner <mattst88 at gmail.com>

I might add something like:

v2: (by Matt Turner) Make JIP/UIP functions take a signed argument.
    (and anything else you did - I think that's the main thing)

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org> [v1]
Reviewed-by: Matt Turner <mattst88 at gmail.com> [v1]
Signed-off-by: Matt Turner <mattst88 at gmail.com> [v2]

[snip]
> +/**
> + * Flow control instruction bits:
> + *  @{
> + */
> +static inline void
> +brw_inst_set_uip(const struct brw_context *brw,
> +                 struct brw_instruction *inst, int16_t value)

int16_t makes sense for Gen4-7, but won't work well for Gen8+.  On Broadwell, 
you have to multiply the number of instructions by 16, instead of 2 on Gen5-7, 
or 1 on Gen4.  With a 16-bit integer, this limits you to 2^15 / 16 = 2047 
instruction jump distances.  We've definitely had shaders which would exceed 
that.

Broadwell uses a full 32-bit value, which gives you 2^31 / 16 = 134217728 
instruction jump distances - more than enough.

I think the cleanest way to handle this is to make the parameters int32_t and 
do this:

static inline void
brw_inst_set_uip(const struct brw_context *brw,
                 struct brw_instruction *inst, int32_t value)
{
   assert(brw->gen >= 6);

   if (brw->gen >= 8) {
      brw_inst_set_bits(inst, 127, 112, (uint16_t) value);
   } else {
      brw_inst_set_bits(inst, 95, 64, (uint32_t) value);
   }
}

As an example...if you start with int16_t jump = -20 (0xffec)...

- Calling this function promotes it to int32_t (0xffffffec).
- Casting it to uint16_t truncates it to     0xffec.
- Casting it to uint32_t truncates it to 0xffffffec.
- Calling brw_inst_set_bits promotes the uint16_t/uint32_t to uint64_t,
  adding more zeros, not f's: 0x000000000000ffec or 0x00000000ffffffec.

So, I'm pretty sure this does what you want.

--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/20140614/5482c21a/attachment-0001.sig>