[Mesa-stable] [Mesa-dev] [PATCH] i965: Fix JIP to properly skip over unrelated control flow.

[Matt Turner]

On Thu, Nov 19, 2015 at 2:05 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> We've apparently always been botching JIP for sequences such as:

Wrong since Dec 1 2015! Nice find.

>
>    do
>        cmp.f0.0 ...
>        (+f0.0) break
>        ...
>        if
>           ...
>        else
>           ...
>        endif
>        ...
>    while
>
> Normally, UIP is supposed to point to the final destination of the jump,
> while in nested control flow, JIP is supposed to point to the end of the
> current nesting level.  It essentially bounces out of the current nested
> control flow, to an instruction that has a JIP which bounces out another
> level, and so on.
>
> In the above example, when setting JIP for the BREAK, we call
> brw_find_next_block_end(), which begins a search after the BREAK for the
> next ENDIF, ELSE, WHILE, or HALT.  It ignores the IF and finds the ELSE,
> setting JIP there.
>
> This makes no sense at all.  The break is supposed to skip over the
> whole if/else/endif block entirely.  They have a sibling relationship,
> not a nesting relationship.
>
> This patch fixes brw_find_next_block_end() to track depth as it does
> its search, and ignore anything not at depth 0.  So when it sees the
> IF, it ignores everything until after the ENDIF.  That way, it finds
> the end of the right block.
>
> Caught while debugging a tessellation shader - no apparent effect on
> Piglit.  I did look for actual applications that were affected, and
> found that GLBenchmark Manhattan had a BREAK with a bogus JIP.
>
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_eu_emit.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index da1ddfd..e457fd2 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -2604,17 +2604,27 @@ brw_find_next_block_end(struct brw_codegen *p, int start_offset)
>     void *store = p->store;
>     const struct brw_device_info *devinfo = p->devinfo;
>
> +   int depth = 0;
> +
>     for (offset = next_offset(devinfo, store, start_offset);
>          offset < p->next_insn_offset;
>          offset = next_offset(devinfo, store, offset)) {
>        brw_inst *insn = store + offset;
>
>        switch (brw_inst_opcode(devinfo, insn)) {
> +      case BRW_OPCODE_IF:
> +         depth++;
> +         break;
>        case BRW_OPCODE_ENDIF:
> +         if (depth == 0)
> +            return offset;
> +         depth--;
> +         break;
>        case BRW_OPCODE_ELSE:
>        case BRW_OPCODE_WHILE:
>        case BRW_OPCODE_HALT:
> -        return offset;
> +         if (depth == 0)
> +            return offset;


Reviewed-by: Matt Turner <mattst88 at gmail.com>