On Thu, Apr 17, 2014 at 2:25 PM, Kenneth Graunke <kenn...@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.
>> +}
>> +
>> +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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
