[Mesa-dev] [PATCH 5/8] i965: Prepare the uip/jip setting for compacted instructions in the program.

[Paul Berry]

On 31 August 2012 11:32, Eric Anholt <eric at anholt.net> wrote:

> The first cut at instruction compaction won't compact things that
> would change control flow jump distances, but we do need to still be
> able to walk the instruction stream, which involves jumping by 8 or 16
> bytes between instructions.
>

I'm not thrilled by the fact that after instruction compaction, p->store
still has type struct brw_instruction *, but the data it points to doesn't
always make sense as an array of struct brw_instruction.  It seems like
there's a risk that we will later come along and add a p->store[...]
reference somewhere it doesn't belong, and get ourselves into trouble.  But
I don't really have a better suggestion to offer.


> ---
>  src/mesa/drivers/dri/i965/brw_eu_emit.c |   57
> +++++++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 4d7b76d..c36742a 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -2476,12 +2476,24 @@ void brw_urb_WRITE(struct brw_compile *p,
>  }
>
>  static int
> +next_ip(struct brw_compile *p, int ip)
> +{
> +   struct brw_instruction *insn = (void *)p->store + ip;
> +
> +   if (insn->header.cmpt_control)
> +      return ip + 8;
> +   else
> +      return ip + 16;
> +}
> +
> +static int
>  brw_find_next_block_end(struct brw_compile *p, int start)
>  {
>     int ip;
> +   void *store = p->store;
>
> -   for (ip = start + 1; ip < p->nr_insn; ip++) {
> -      struct brw_instruction *insn = &p->store[ip];
> +   for (ip = next_ip(p, start); ip < p->next_insn_offset; ip = next_ip(p,
> ip)) {
> +      struct brw_instruction *insn = store + ip;
>
>        switch (insn->header.opcode) {
>        case BRW_OPCODE_ENDIF:
> @@ -2503,20 +2515,24 @@ brw_find_loop_end(struct brw_compile *p, int start)
>  {
>     struct intel_context *intel = &p->brw->intel;
>     int ip;
> -   int br = 2;
> +   int scale = 8;
> +   void *store = p->store;
>
> -   for (ip = start + 1; ip < p->nr_insn; ip++) {
> -      struct brw_instruction *insn = &p->store[ip];
> +   /* Always start after the instruction (such as a WHILE) we're trying
> to fix
> +    * up.
> +    */
> +   for (ip = next_ip(p, start); ip < p->next_insn_offset; ip = next_ip(p,
> ip)) {
> +      struct brw_instruction *insn = store + ip;
>
>        if (insn->header.opcode == BRW_OPCODE_WHILE) {
>          int jip = intel->gen == 6 ? insn->bits1.branch_gen6.jump_count
>                                    : insn->bits3.break_cont.jip;
> -        if (ip + jip / br <= start)
> +        if (ip + jip * scale <= start)
>             return ip;
>        }
>     }
>     assert(!"not reached");
> -   return start + 1;
> +   return start;
>  }
>
>  /* After program generation, go back and update the UIP and JIP of
> @@ -2527,24 +2543,37 @@ brw_set_uip_jip(struct brw_compile *p)
>  {
>     struct intel_context *intel = &p->brw->intel;
>     int ip;
> -   int br = 2;
> +   int scale = 8;
> +   void *store = p->store;
>
>     if (intel->gen < 6)
>        return;
>
> -   for (ip = 0; ip < p->nr_insn; ip++) {
> -      struct brw_instruction *insn = &p->store[ip];
> +   for (ip = 0; ip < p->next_insn_offset; ip = next_ip(p, ip)) {
> +      struct brw_instruction *insn = store + ip;
> +
> +      if (insn->header.cmpt_control) {
> +        /* Fixups for compacted BREAK/CONTINUE not supported yet. */
> +        assert(insn->header.opcode != BRW_OPCODE_BREAK &&
> +               insn->header.opcode != BRW_OPCODE_CONTINUE &&
> +               insn->header.opcode != BRW_OPCODE_HALT);
> +        continue;
> +      }
>
>        switch (insn->header.opcode) {
>        case BRW_OPCODE_BREAK:
> -        insn->bits3.break_cont.jip = br * (brw_find_next_block_end(p, ip)
> - ip);
> +        insn->bits3.break_cont.jip =
> +            (brw_find_next_block_end(p, ip) - ip) / scale;
>          /* Gen7 UIP points to WHILE; Gen6 points just after it */
>          insn->bits3.break_cont.uip =
> -           br * (brw_find_loop_end(p, ip) - ip + (intel->gen == 6 ? 1 :
> 0));
> +           (brw_find_loop_end(p, ip) - ip +
> +             (intel->gen == 6 ? 16 : 0)) / scale;
>

It looks from patch 7/8 like the WHILE instruction is never compacted, so I
believe this is correct.  However, if we ever decide to compact the WHILE
instruction this will be wrong for gen6.  Would it be possible to add an
assertion here (or at the very least a comment) to alert us to the fact
that this will need to be fixed when we get around to compacting WHILE?


>          break;
>        case BRW_OPCODE_CONTINUE:
> -        insn->bits3.break_cont.jip = br * (brw_find_next_block_end(p, ip)
> - ip);
> -        insn->bits3.break_cont.uip = br * (brw_find_loop_end(p, ip) - ip);
> +        insn->bits3.break_cont.jip =
> +            (brw_find_next_block_end(p, ip) - ip) / scale;
> +        insn->bits3.break_cont.uip =
> +            (brw_find_loop_end(p, ip) - ip) / scale;
>
>          assert(insn->bits3.break_cont.uip != 0);
>          assert(insn->bits3.break_cont.jip != 0);
> --
> 1.7.10.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120904/749086a9/attachment.html>