[Mesa-dev,26/32] i965/vec4: Don't assume a value is dead when its VGRF is only partially overwritten.

Submitted by Francisco Jerez on Feb. 6, 2015, 2:43 p.m.

Details

Message ID 1423233792-11767-26-git-send-email-currojerez@riseup.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Feb. 6, 2015, 2:43 p.m.
---
 src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
index 9604e60..5df0d31 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
@@ -122,7 +122,8 @@  vec4_visitor::dead_code_eliminate()
             }
          }
 
-         if (inst->dst.file == GRF && !inst->predicate) {
+         if (inst->dst.file == GRF && !inst->predicate &&
+             inst->regs_written == alloc.sizes[inst->dst.reg]) {
             for (int c = 0; c < 4; c++) {
                if (inst->dst.writemask & (1 << c)) {
                   int var = inst->dst.reg * 4 + c;

Comments

On Fri, Feb 6, 2015 at 6:43 AM, Francisco Jerez <currojerez@riseup.net> wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> index 9604e60..5df0d31 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
> @@ -122,7 +122,8 @@ vec4_visitor::dead_code_eliminate()
>              }
>           }
>
> -         if (inst->dst.file == GRF && !inst->predicate) {
> +         if (inst->dst.file == GRF && !inst->predicate &&
> +             inst->regs_written == alloc.sizes[inst->dst.reg]) {
>              for (int c = 0; c < 4; c++) {
>                 if (inst->dst.writemask & (1 << c)) {
>                    int var = inst->dst.reg * 4 + c;
> --
> 2.1.3

I think what you're saying is that if an instruction wrote less than
the size of the destination register we would incorrectly mark it as
entirely overwritten?

I'm not reading the code that way. It looks like we're just marking
individual channels (i.e., channels of the writemask) as dead. I don't
see, for example, us using the size of the destination register
anywhere in the loop.

How you triggered this, again, would be helpful.
Matt Turner <mattst88@gmail.com> writes:

> On Fri, Feb 6, 2015 at 6:43 AM, Francisco Jerez <currojerez@riseup.net> wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
>> index 9604e60..5df0d31 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
>> @@ -122,7 +122,8 @@ vec4_visitor::dead_code_eliminate()
>>              }
>>           }
>>
>> -         if (inst->dst.file == GRF && !inst->predicate) {
>> +         if (inst->dst.file == GRF && !inst->predicate &&
>> +             inst->regs_written == alloc.sizes[inst->dst.reg]) {
>>              for (int c = 0; c < 4; c++) {
>>                 if (inst->dst.writemask & (1 << c)) {
>>                    int var = inst->dst.reg * 4 + c;
>> --
>> 2.1.3
>
> I think what you're saying is that if an instruction wrote less than
> the size of the destination register we would incorrectly mark it as
> entirely overwritten?
>
Yes, exactly.

> I'm not reading the code that way. It looks like we're just marking
> individual channels (i.e., channels of the writemask) as dead. I don't
> see, for example, us using the size of the destination register
> anywhere in the loop.
>
> How you triggered this, again, would be helpful.

The problem is that the current dead code elimination code doesn't take
into account that VGRFs could be larger than one register (and, ugh,
neither does the VEC4 live interval calculation code), so it will think
that a value spanning several registers is dead after any of the
registers is overwritten.

I started fixing the live intervals code and got distracted by something
higher-priority while I was in the middle of fixing the million
assumptions I broke by changing the granularity of the intervals.  In
the meantime this should keep dead code eliminate from eliminating live
instructions that write to several registers.