Kenneth Graunke <kenn...@whitecape.org> writes: > On Saturday, January 14, 2017 11:09:53 PM PST Francisco Jerez wrote: >> Hi Ken! >> >> Kenneth Graunke <kenn...@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 <kenn...@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 <curroje...@riseup.net>
>>
>> > !inst->flags_written() &&
>> > !inst->writes_accumulator) {
>> > inst->opcode = BRW_OPCODE_NOP;
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
