[Mesa-dev] [PATCH 10/11] i965/vs: Simplify the scratch write handling and emit less code.

[Kenneth Graunke]

On 10/04/2012 04:07 PM, Eric Anholt wrote:
> We're always doing it for an immediately preceding instruction, and we can
> make that instruction write directly into the MRF instead of using a
> temporary.
>
> Along with reducing instructions, it avoids confusing the upcoming live
> variables code (which saw our use of all channels of the temporary that had
> only one channel populated, and assumes the register was live at the start of
> the basic block, i.e. main()).
> ---
>   src/mesa/drivers/dri/i965/brw_vec4.h               |    2 --
>   src/mesa/drivers/dri/i965/brw_vec4_emit.cpp        |    8 ++++---
>   .../drivers/dri/i965/brw_vec4_reg_allocate.cpp     |   11 +---------
>   src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp     |   23 +++++++++-----------
>   4 files changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> index cd05462..fe70300 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> @@ -419,8 +419,6 @@ public:
>   			  src_reg orig_src,
>   			  int base_offset);
>      void emit_scratch_write(vec4_instruction *inst,
> -			   src_reg temp,
> -			   dst_reg orig_dst,
>   			   int base_offset);
>      void emit_pull_constant_load(vec4_instruction *inst,
>   				dst_reg dst,
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp b/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp
> index 479b0a6..340841f 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp
> @@ -588,9 +588,11 @@ vec4_visitor::generate_scratch_write(vec4_instruction *inst,
>      generate_oword_dual_block_offsets(brw_message_reg(inst->base_mrf + 1),
>   				     index);
>
> -   brw_MOV(p,
> -	   retype(brw_message_reg(inst->base_mrf + 2), BRW_REGISTER_TYPE_D),
> -	   retype(src, BRW_REGISTER_TYPE_D));
> +   if (src.file != BRW_ARCHITECTURE_REGISTER_FILE || src.nr != BRW_ARF_NULL) {
> +      brw_MOV(p,
> +              retype(brw_message_reg(inst->base_mrf + 2), BRW_REGISTER_TYPE_D),
> +              retype(src, BRW_REGISTER_TYPE_D));
> +   }
>
>      uint32_t msg_type;
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> index 5529794..79966e8 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_reg_allocate.cpp
> @@ -346,16 +346,7 @@ vec4_visitor::spill_reg(int spill_reg_nr)
>         }
>
>         if (inst->dst.file == GRF && inst->dst.reg == spill_reg_nr) {
> -         dst_reg spill_reg = inst->dst;
> -         inst->dst.reg = virtual_grf_alloc(1, 4);
> -
> -         /* We don't want a swizzle when reading from the source; read the
> -          * whole register and use spill_reg's writemask to select which
> -          * channels to write.
> -          */
> -         src_reg temp = src_reg(inst->dst);
> -         temp.swizzle = BRW_SWIZZLE_XYZW;

I don't understand how you can remove this.  You need to write out 
inst->dst, but non-swizzled.  (Otherwise you re-swizzle your swizzles 
and get the wrong channels...)

Enable the "Go spill everything" debugging on brw_vec4_emit.cpp:817 and 
then run:

./bin/shader_runner tests/shaders/glsl-vs-masked-cos.shader_test -auto

Your patch causes an assertion failure, which is a regression:
brw_eu_emit.c:1672: brw_math: Assertion `dest.file == 1' failed.

> -         emit_scratch_write(inst, temp, spill_reg, spill_offset);
> +         emit_scratch_write(inst, spill_offset);
>         }
>      }
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index 762f4c1..1dfdcce 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -2468,19 +2468,23 @@ vec4_visitor::emit_scratch_read(vec4_instruction *inst,
>    */
>   void
>   vec4_visitor::emit_scratch_write(vec4_instruction *inst,
> -				 src_reg temp, dst_reg orig_dst,
>   				 int base_offset)
>   {
> -   int reg_offset = base_offset + orig_dst.reg_offset;
> -   src_reg index = get_scratch_offset(inst, orig_dst.reladdr, reg_offset);
> +   int reg_offset = base_offset + inst->dst.reg_offset;
> +   src_reg index = get_scratch_offset(inst, inst->dst.reladdr, reg_offset);
>
>      dst_reg dst = dst_reg(brw_writemask(brw_vec8_grf(0, 0),
> -				       orig_dst.writemask));
> -   vec4_instruction *write = SCRATCH_WRITE(dst, temp, index);
> +				       inst->dst.writemask));
> +   vec4_instruction *write = SCRATCH_WRITE(dst, src_reg(), index);

BAD_FILE?  How can this work?

>      write->predicate = inst->predicate;
>      write->ir = inst->ir;
>      write->annotation = inst->annotation;
>      inst->insert_after(write);
> +
> +   inst->dst.file = MRF;
> +   inst->dst.reg = write->base_mrf + 2;
> +   inst->dst.reg_offset = 0;
> +   inst->dst.reladdr = NULL;
>   }
>
>   /**
> @@ -2535,14 +2539,7 @@ vec4_visitor::move_grf_array_access_to_scratch()
>         current_annotation = inst->annotation;
>
>         if (inst->dst.file == GRF && scratch_loc[inst->dst.reg] != -1) {
> -	 src_reg temp = src_reg(this, glsl_type::vec4_type);
> -
> -	 emit_scratch_write(inst, temp, inst->dst, scratch_loc[inst->dst.reg]);
> -
> -	 inst->dst.file = temp.file;
> -	 inst->dst.reg = temp.reg;
> -	 inst->dst.reg_offset = temp.reg_offset;
> -	 inst->dst.reladdr = NULL;
> +	 emit_scratch_write(inst, scratch_loc[inst->dst.reg]);
>         }
>
>         for (int i = 0 ; i < 3; i++) {
>