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

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> 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.
>

I doubt MRFs would prevent the scheduler from moving a flag-consuming
operation past the FB write.

> 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!
>

Heh, fair enough.  With that fixed [and the comment in the commit
message saying that this is only a code cleanup ;)] patch is:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>> 
>> >               !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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170117/aa83b610/attachment.sig>