[Mesa-dev] [PATCH 2/6] i965/fs: Recognize nop-MOV instructions early.

[Kenneth Graunke]

On 04/17/2014 02:45 PM, Matt Turner wrote:
> On Thu, Apr 17, 2014 at 2:25 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> On 04/16/2014 11:07 AM, Matt Turner wrote:
>>> And avoid rewriting other instructions unnecessarily. Removes a few
>>> self-moves we weren't able to handle because they were components of a
>>> large VGRF.
>>>
>>> instructions in affected programs:     830 -> 826 (-0.48%)
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp | 16 +++++++++++++---
>>>  1 file changed, 13 insertions(+), 3 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 f6d9b68..8b37ed0 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>>> @@ -44,6 +44,12 @@
>>>  #include "brw_fs_live_variables.h"
>>>
>>>  static bool
>>> +is_nop_mov(const fs_inst *inst)
>>> +{
>>> +   return inst->dst.equals(inst->src[0]);
>>
>> It feels weird having a function called is_nop_mov that doesn't ensure
>> that inst->opcode == BRW_OPCODE_MOV.  I know it's unnecessary, since
>> this function is only called after is_coalesce_candidate(), but...it
>> might be nice to add that check anyway, or maybe a comment or assertion.
> 
> Sure, I kind of do that when I add SHADER_OPCODE_LOAD_PAYLOAD in the
> next series. I can do it now if it's important.

I would prefer to see that - it's an easy change, and makes the function
more robust.

> 
>>> +}
>>> +
>>> +static bool
>>>  is_coalesce_candidate(const fs_inst *inst, const int *virtual_grf_sizes)
>>>  {
>>>     if (inst->opcode != BRW_OPCODE_MOV ||
>>> @@ -70,9 +76,7 @@ can_coalesce_vars(brw::fs_live_variables *live_intervals,
>>>                    const exec_list *instructions, const fs_inst *inst,
>>>                    int var_to, int var_from)
>>>  {
>>> -   if (live_intervals->vars_interfere(var_from, var_to) &&
>>> -       !inst->dst.equals(inst->src[0])) {
>>> -
>>> +   if (live_intervals->vars_interfere(var_from, var_to)) {
>>>        /* We know that the live ranges of A (var_from) and B (var_to)
>>>         * interfere because of the ->vars_interfere() call above. If the end
>>>         * of B's live range is after the end of A's range, then we know two
>>> @@ -131,6 +135,12 @@ fs_visitor::register_coalesce()
>>>        if (!is_coalesce_candidate(inst, virtual_grf_sizes))
>>>           continue;
>>>
>>> +      if (is_nop_mov(inst)) {
>>> +         inst->opcode = BRW_OPCODE_NOP;
>>> +         progress = true;
>>> +         continue;
>>> +      }
>>
>> Having the can_coalesce_vars function alter the instruction stream also
>> seems strange.  What do you think about moving the MOV->NOP code into
>> the caller?  It would be outside the vars_interfere path, but I think
>> that's okay...
> 
> can_coalesce_vars() doesn't modify the instruction stream....?
> 
> I'm not sure what you mean, but I'll try to explain. Checking for
> self-moves here, rather than in can_coalesce_vars() allows us to
> remove self-moves of single components of a large VGRF.
> 
> mov vgrf4+1, vgrf4+1
> 
> for instance isn't handled by the rest of register coalescing if other
> bits of vgrf4+1 can't be coalesced.
> 

I'm sorry - I misread the diff.  I thought you added this new hunk in
the can_coalesce_vars function, which is a boolean predicate (and
shouldn't actually /change/ things).  But you didn't - you put it in
register_coalesce().  What you did makes sense.

Patches 1-3 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
(with or without changes)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140417/37e1bf74/attachment.sig>