[Mesa-dev] [PATCH 2/2] tgsi/scan: correctly walk instructions in tgsi_scan_tess_ctrl()

[Marek Olšák]

For the series:

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek

On Fri, Dec 14, 2018 at 12:33 AM Timothy Arceri <tarceri at itsqueeze.com>
wrote:

> The previous code used a do while loop and continues after walking
> a nested loop/if-statement. This means we end up evaluating the
> last instruction from the nested block against the while condition
> and potentially exit early if it matches the exit condition of the
> outer block.
>
> Fixes: 386d165d8d09 ("tgsi/scan: add a new pass that analyzes tess factor
> writes")
> ---
> No changes in shader-db. Didn't run against piglit.
>
>  src/gallium/auxiliary/tgsi/tgsi_scan.c | 72 +++++++++++++++-----------
>  1 file changed, 43 insertions(+), 29 deletions(-)
>
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_scan.c
> b/src/gallium/auxiliary/tgsi/tgsi_scan.c
> index 6e2e23822c..d56844bdc2 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_scan.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_scan.c
> @@ -1028,11 +1028,12 @@ get_block_tessfactor_writemask(const struct
> tgsi_shader_info *info,
>     struct tgsi_full_instruction *inst;
>     unsigned writemask = 0;
>
> -   do {
> -      tgsi_parse_token(parse);
> -      assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> -      inst = &parse->FullToken.FullInstruction;
> -      check_no_subroutines(inst);
> +   tgsi_parse_token(parse);
> +   assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> +   inst = &parse->FullToken.FullInstruction;
> +   check_no_subroutines(inst);
> +
> +   while (inst->Instruction.Opcode != end_opcode) {
>
>        /* Recursively process nested blocks. */
>        switch (inst->Instruction.Opcode) {
> @@ -1040,20 +1041,26 @@ get_block_tessfactor_writemask(const struct
> tgsi_shader_info *info,
>        case TGSI_OPCODE_UIF:
>           writemask |=
>              get_block_tessfactor_writemask(info, parse,
> TGSI_OPCODE_ENDIF);
> -         continue;
> +         break;
>
>        case TGSI_OPCODE_BGNLOOP:
>           writemask |=
>              get_block_tessfactor_writemask(info, parse,
> TGSI_OPCODE_ENDLOOP);
> -         continue;
> +         break;
>
>        case TGSI_OPCODE_BARRIER:
>           unreachable("nested BARRIER is illegal");
> -         continue;
> +         break;
> +
> +      default:
> +         writemask |= get_inst_tessfactor_writemask(info, inst);
>        }
>
> -      writemask |= get_inst_tessfactor_writemask(info, inst);
> -   } while (inst->Instruction.Opcode != end_opcode);
> +      tgsi_parse_token(parse);
> +      assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> +      inst = &parse->FullToken.FullInstruction;
> +      check_no_subroutines(inst);
> +   }
>
>     return writemask;
>  }
> @@ -1067,18 +1074,20 @@ get_if_block_tessfactor_writemask(const struct
> tgsi_shader_info *info,
>     struct tgsi_full_instruction *inst;
>     unsigned then_tessfactor_writemask = 0;
>     unsigned else_tessfactor_writemask = 0;
> +   unsigned writemask;
>     bool is_then = true;
>
> -   do {
> -      tgsi_parse_token(parse);
> -      assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> -      inst = &parse->FullToken.FullInstruction;
> -      check_no_subroutines(inst);
> +   tgsi_parse_token(parse);
> +   assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> +   inst = &parse->FullToken.FullInstruction;
> +   check_no_subroutines(inst);
> +
> +   while (inst->Instruction.Opcode != TGSI_OPCODE_ENDIF) {
>
>        switch (inst->Instruction.Opcode) {
>        case TGSI_OPCODE_ELSE:
>           is_then = false;
> -         continue;
> +         break;
>
>        /* Recursively process nested blocks. */
>        case TGSI_OPCODE_IF:
> @@ -1087,28 +1096,33 @@ get_if_block_tessfactor_writemask(const struct
> tgsi_shader_info *info,
>                                             is_then ?
> &then_tessfactor_writemask :
>
> &else_tessfactor_writemask,
>                                             cond_block_tf_writemask);
> -         continue;
> +         break;
>
>        case TGSI_OPCODE_BGNLOOP:
>           *cond_block_tf_writemask |=
>              get_block_tessfactor_writemask(info, parse,
> TGSI_OPCODE_ENDLOOP);
> -         continue;
> +         break;
>
>        case TGSI_OPCODE_BARRIER:
>           unreachable("nested BARRIER is illegal");
> -         continue;
> -      }
> -
> -      /* Process an instruction in the current block. */
> -      unsigned writemask = get_inst_tessfactor_writemask(info, inst);
> +         break;
> +      default:
> +         /* Process an instruction in the current block. */
> +         writemask = get_inst_tessfactor_writemask(info, inst);
>
> -      if (writemask) {
> -         if (is_then)
> -            then_tessfactor_writemask |= writemask;
> -         else
> -            else_tessfactor_writemask |= writemask;
> +         if (writemask) {
> +            if (is_then)
> +               then_tessfactor_writemask |= writemask;
> +            else
> +               else_tessfactor_writemask |= writemask;
> +         }
>        }
> -   } while (inst->Instruction.Opcode != TGSI_OPCODE_ENDIF);
> +
> +      tgsi_parse_token(parse);
> +      assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> +      inst = &parse->FullToken.FullInstruction;
> +      check_no_subroutines(inst);
> +   }
>
>     if (then_tessfactor_writemask || else_tessfactor_writemask) {
>        /* If both statements write the same tess factor channels,
> --
> 2.19.2
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181221/ca6c1e49/attachment-0001.html>