[Mesa-dev] [PATCH 14/32] i965/fs: Fix register coalesce not to lose track of the second half of 16-wide moves.

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
>> On Fri, Feb 6, 2015 at 6:42 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> Fixes rewrite by the register coalesce pass of references to
>>> individual halves of 16-wide coalesced registers.
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp b/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>>> index 09f0fad..2a26a46 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>>> @@ -211,9 +211,13 @@ fs_visitor::register_coalesce()
>>>              continue;
>>>           }
>>>           reg_to_offset[offset] = inst->dst.reg_offset;
>>> -         if (inst->src[0].width == 16)
>>> -            reg_to_offset[offset + 1] = inst->dst.reg_offset + 1;
>>>           mov[offset] = inst;
>>> +
>>> +         if (inst->exec_size * type_sz(inst->src[0].type) > REG_SIZE) {
>>
>> Why are we not using "inst->regs_read(0) > 1"?
>>
> Yeah, that would work too, and is certainly more readable, I'll change
> it.
>
Actually upon re-reading the register coalesce pass I just cannot make
sense of the rewrite loop at the bottom...  Why does it need to loop for
each sub-register of the VGRF once for each instruction?  Wouldn't some
simple arithmetic be sufficient to find out whether a source or
destination overlaps with the coalesced register?  And why does it even
check that the movs are non-NULL?  Isn't it guaranteed because of the
previous checks that the whole register has been copied?

Bah...  How about we drop this patch (except for the line that changes
the condition in the if statement -- that was probably a bug of its own)
and fix the rewrite loop instead?

>>> +            reg_to_offset[offset + 1] = inst->dst.reg_offset + 1;
>>> +            mov[offset + 1] = inst;
>>> +         }
>>> +
>>>           channels_remaining -= inst->regs_written;
>>>        }
>>>
>>> --
>>> 2.1.3
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150728/c82999e5/attachment.sig>