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

2018-12-21 Thread Marek Olšák 
For the series:

Reviewed-by: Marek Olšák 

Marek

On Fri, Dec 14, 2018 at 12:33 AM Timothy Arceri 
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 = >FullToken.FullInstruction;
> -  check_no_subroutines(inst);
> +   tgsi_parse_token(parse);
> +   assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> +   inst = >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 = >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 = >FullToken.FullInstruction;
> -  check_no_subroutines(inst);
> +   tgsi_parse_token(parse);
> +   assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
> +   inst = >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 ?
> _tessfactor_writemask :
>
> _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

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

2018-12-13 Thread Timothy Arceri 
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 = >FullToken.FullInstruction;
-  check_no_subroutines(inst);
+   tgsi_parse_token(parse);
+   assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
+   inst = >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 = >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 = >FullToken.FullInstruction;
-  check_no_subroutines(inst);
+   tgsi_parse_token(parse);
+   assert(parse->FullToken.Token.Type == TGSI_TOKEN_TYPE_INSTRUCTION);
+   inst = >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 ? 
_tessfactor_writemask :
  
_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 = >FullToken.FullInstruction;
+  check_no_subroutines(inst);
+   }
 
if (then_tessfactor_writemask || else_tessfactor_writemask) {