[Mesa-dev] [PATCH 7/8] i965: call next_insn() before referencing a instruction by index

[Kenneth Graunke]

On 12/21/2011 01:33 AM, Yuanhan Liu wrote:
[snip]
> +   int emit_endif = 1;

Please use bool and true/false rather than int.

>     /* In single program flow mode, we can express IF and ELSE instructions
>      * equivalently as ADD instructions that operate on IP.  On platforms prior
> @@ -1219,14 +1211,32 @@ brw_ENDIF(struct brw_compile *p)
>      * instructions to conditional ADDs.  So we only do this trick on Gen4 and
>      * Gen5.
>      */
> -   if (intel->gen < 6 && p->single_program_flow) {
> +   if (intel->gen < 6 && p->single_program_flow)
> +      emit_endif = 0;

You could actually just do this:

/* In single program flow mode, we can express IF and ELSE ...........
 */
bool emit_endif = !(intel->gen < 6 && p->single_program_flow);

But I'm fine with "bool emit_endif = true" and "emit_endif = false" if
you prefer that.

Assuming you make one of those changes, this patch is
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +   /*
> +    * A single next_insn() may change the base adress of instruction store
> +    * memory(p->store), so call it first before referencing the instruction
> +    * store pointer from an index
> +    */
> +   if (emit_endif)
> +      insn = next_insn(p, BRW_OPCODE_ENDIF);
> +
> +   /* Pop the IF and (optional) ELSE instructions from the stack */
> +   p->if_depth_in_loop[p->loop_stack_depth]--;
> +   tmp = pop_if_stack(p);
> +   if (tmp->header.opcode == BRW_OPCODE_ELSE) {
> +      else_inst = tmp;
> +      tmp = pop_if_stack(p);
> +   }
> +   if_inst = tmp;
> +
> +   if (!emit_endif) {
>        /* ENDIF is useless; don't bother emitting it. */
>        convert_IF_ELSE_to_ADD(p, if_inst, else_inst);
>        return;
>     }
>  
> -   insn = next_insn(p, BRW_OPCODE_ENDIF);
> -
>     if (intel->gen < 6) {
>        brw_set_dest(p, insn, retype(brw_vec4_grf(0,0), BRW_REGISTER_TYPE_UD));
>        brw_set_src0(p, insn, retype(brw_vec4_grf(0,0), BRW_REGISTER_TYPE_UD));
> @@ -1393,13 +1403,12 @@ struct brw_instruction *brw_WHILE(struct brw_compile *p)
>     struct brw_instruction *insn, *do_insn;
>     GLuint br = 1;
>  
> -   do_insn = get_inner_do_insn(p);
> -
>     if (intel->gen >= 5)
>        br = 2;
>  
>     if (intel->gen >= 7) {
>        insn = next_insn(p, BRW_OPCODE_WHILE);
> +      do_insn = get_inner_do_insn(p);
>  
>        brw_set_dest(p, insn, retype(brw_null_reg(), BRW_REGISTER_TYPE_D));
>        brw_set_src0(p, insn, retype(brw_null_reg(), BRW_REGISTER_TYPE_D));
> @@ -1409,6 +1418,7 @@ struct brw_instruction *brw_WHILE(struct brw_compile *p)
>        insn->header.execution_size = BRW_EXECUTE_8;
>     } else if (intel->gen == 6) {
>        insn = next_insn(p, BRW_OPCODE_WHILE);
> +      do_insn = get_inner_do_insn(p);
>  
>        brw_set_dest(p, insn, brw_imm_w(0));
>        insn->bits1.branch_gen6.jump_count = br * (do_insn - insn);
> @@ -1419,6 +1429,7 @@ struct brw_instruction *brw_WHILE(struct brw_compile *p)
>     } else {
>        if (p->single_program_flow) {
>  	 insn = next_insn(p, BRW_OPCODE_ADD);
> +         do_insn = get_inner_do_insn(p);
>  
>  	 brw_set_dest(p, insn, brw_ip_reg());
>  	 brw_set_src0(p, insn, brw_ip_reg());
> @@ -1426,6 +1437,7 @@ struct brw_instruction *brw_WHILE(struct brw_compile *p)
>  	 insn->header.execution_size = BRW_EXECUTE_1;
>        } else {
>  	 insn = next_insn(p, BRW_OPCODE_WHILE);
> +         do_insn = get_inner_do_insn(p);
>  
>  	 assert(do_insn->header.opcode == BRW_OPCODE_DO);
>