[Mesa-dev] [PATCH] i965/vec4: Only zero out unused message components when there are any.

[Ian Romanick]

On 09/09/2013 03:35 PM, Ian Romanick wrote:
> On 09/09/2013 01:38 PM, Kenneth Graunke wrote:
>> Otherwise, coordinates with four components would result in a MOV
>> with a destination writemask that has no channels enabled:
>>
>> mov(8) g115<1>.F 0D { align16 WE_normal NoDDChk 1Q };
>>
>> At best, this is stupid: we emit code that shouldn't do anything.
>> Worse, it apparently causes GPU hangs (observable with Chris's
>> textureGather test on CubeArrays.)

I also meant to say:

Do we have something like ir_validate for GEN assembly?  It sure seems
like we should...

>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> Cc: Chris Forbes <chrisf at ijw.co.nz>
>> Cc: mesa-stable at lists.freedesktop.org
>> ---
>>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> index ca52fd3..8cfe4e3 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>> @@ -2245,8 +2245,10 @@ vec4_visitor::visit(ir_texture *ir)
>>  	 emit(MOV(dst_reg(MRF, param_base, ir->coordinate->type, coord_mask),
>>  		  coordinate));
>>        }
>> -      emit(MOV(dst_reg(MRF, param_base, ir->coordinate->type, zero_mask),
>> -	       src_reg(0)));
>> +      if (zero_mask != 0) {
>> +         emit(MOV(dst_reg(MRF, param_base, ir->coordinate->type, zero_mask),
>> +                  src_reg(0)));
>> +      }
> 
> I'd be tempted to do this as:
> 
>     if (coord_mask != 0x0f) {
>        const int zero_mask = 0x0f & ~coord_mask;
> 
>        emit(MOV(dst_reg(MRF, param_base, ir->coordinate->type, zero_mask),
>                 src_reg(0)));
>     }
> 
> And delete the silly zero_mask loop above.
> 
> I'm also not sure why coord_mask is calculated by a loop, but I can
> leave that bikeshed for another day...
> 
> Either way, the patch is
> 
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
> 
>>        /* Load the shadow comparitor */
>>        if (ir->shadow_comparitor && ir->op != ir_txd) {
>>  	 emit(MOV(dst_reg(MRF, param_base + 1, ir->shadow_comparitor->type,
>