[Mesa-dev] [PATCH v2 03/30] i965/fs: Fix copy propagation of load payload for double operands

[Samuel Iglesias Gonsálvez]

On 13/05/16 01:52, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
>> From: Iago Toral Quiroga <itoral at igalia.com>
>>
>> Specifically, consider the size of the data type of the operand to compute
>> the number of registers written.
>>
>> v2 (Sam):
>> - Fix line width (Jordan).
>> - Add an assert (Jordan).
>> - Use REG_SIZE in the calculation of regs_written (Curro)
>>
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> index 83791bf..fd423a3 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>> @@ -774,8 +774,10 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, bblock_t *block,
>>                   inst->dst.file == VGRF) {
>>           int offset = 0;
>>           for (int i = 0; i < inst->sources; i++) {
>> +            assert(type_sz(inst->src[i].type) >= 4);
> 
> The assertion doesn't look right to me, if you had taken my suggestion
> into account the code below wouldn't care what the type size is as long
> as:
> 
> | assert(effective_width * type_sz(...) % REG_SIZE == 0);
> 

OK, I am going to replace the assert to this one.

>>              int effective_width = i < inst->header_size ? 8 : inst->exec_size;
>> -            int regs_written = effective_width / 8;
>> +            int regs_written = effective_width / 8 *
>> +               type_sz(inst->src[i].type) / REG_SIZE;
> 
> Sigh, you left the '/ 8' in there, the expression should have been
> 'effective_width * type_sz(inst->src[i].type) / REG_SIZE'.
> 

Oh, I have not seen this. I will fix it now.

With these changes, Does it get your R-b?

Sam

>>              if (inst->src[i].file == VGRF) {
>>                 acp_entry *entry = ralloc(copy_prop_ctx, acp_entry);
>>                 entry->dst = inst->dst;
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev