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

[Francisco Jerez]

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

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

>               !inst->flags_written() &&
>               !inst->writes_accumulator) {
>              inst->opcode = BRW_OPCODE_NOP;
> -- 
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- 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/20170114/76e91128/attachment-0001.sig>