[Mesa-dev] [PATCH 1/2] i965: Combine some dead code elimination NOP'ing code.

[Kenneth Graunke]

On Saturday, January 14, 2017 11:09:53 PM PST Francisco Jerez wrote:
> Hi Ken!
> 
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
> > In theory we might have incorrectly NOP'd instructions that write the
> > flag, but where that flag value isn't used, and yet the instruction
> > either writes the accumulator or has side effects.
> >
> > I don't believe any such instructions exist, so this is mostly a
> > code cleanup.
> >
> One example that occurred to me: The FB write opcode writes the flag
> register (on Gen4-5 due to the dynamic AA payload hack), its flag result
> is invariably dead (because the flag result is only ever used internally
> to decide whether to send AA data or not), and has obvious side effects.
> 
> I believe the only reason why the code below wouldn't incorrectly nop
> out FB writes is because of another, also somewhat concerning bug --
> backend_instruction::flags_written() is completely unaware of FB writes
> modifying the flag register on Gen4-5, which led to actual corruption in
> my SIMD32 branch (I have a somewhat half-baked patch to fix it but I
> think we should probably land this first).

Oh, interesting!  I forgot about that case.  I could see that being a
problem - if there were multiple FB writes (either split up, or MRT),
scheduling might move things around badly.  Presumably the fact that
they use the same MRFs ends up saving us from that, today.

Sounds like this is a worthwhile fix, and we'll need your other ones
as well :)

> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp | 8 +-------
> >  1 file changed, 1 insertion(+), 7 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
> > index 8a0469a51b9..930dc733b45 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
> > @@ -70,17 +70,11 @@ fs_visitor::dead_code_eliminate()
> >              }
> >           }
> >  
> > -         if (inst->dst.is_null() && inst->flags_written()) {
> > -            if (!(flag_live[0] & inst->flags_written())) {
> > -               inst->opcode = BRW_OPCODE_NOP;
> > -               progress = true;
> > -            }
> > -         }
> > -
> >           if ((inst->opcode != BRW_OPCODE_IF &&
> >                inst->opcode != BRW_OPCODE_WHILE) &&
> >               inst->dst.is_null() &&
> >               !inst->has_side_effects() &&
> > +             !(flag_live[0] & inst->flags_written()) &&
> 
> I don't think this will have the intended effect unless you remove the
> line below in addition, otherwise instructions that write a flag result
> which happens to be dead will never get nop'ed out.

Whoops :)  I meant to delete that.  (You can see that it's gone in the
next patch, where I refactored this into a helper function...)  I've
fixed that locally.  Thanks!

> 
> >               !inst->flags_written() &&
> >               !inst->writes_accumulator) {
> >              inst->opcode = BRW_OPCODE_NOP;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170114/97623dcf/attachment.sig>