[Mesa-dev] [PATCH 4/7] i965: Perform basic optimizations on the FIND_LIVE_CHANNEL opcode.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Thursday, April 30, 2015 04:59:49 PM Francisco Jerez wrote:
>> Matt Turner <mattst88 at gmail.com> writes:
>> 
>> > On Fri, Feb 20, 2015 at 11:49 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>> >> ---
>> >>  src/mesa/drivers/dri/i965/brw_fs.cpp       | 49 ++++++++++++++++++++++++++++++
>> >>  src/mesa/drivers/dri/i965/brw_fs.h         |  1 +
>> >>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp   |  1 +
>> >>  src/mesa/drivers/dri/i965/brw_vec4.cpp     | 41 +++++++++++++++++++++++++
>> >>  src/mesa/drivers/dri/i965/brw_vec4.h       |  1 +
>> >>  src/mesa/drivers/dri/i965/brw_vec4_cse.cpp |  1 +
>> >>  6 files changed, 94 insertions(+)
>> >>
>> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> index d567c2b..4537900 100644
>> >> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> >> @@ -2734,6 +2734,54 @@ fs_visitor::compute_to_mrf()
>> >>  }
>> >>
>> >>  /**
>> >> + * Eliminate FIND_LIVE_CHANNEL instructions occurring outside any control
>> >> + * flow.  We could probably do better here with some form of divergence
>> >> + * analysis.
>> >> + */
>> >> +bool
>> >> +fs_visitor::eliminate_find_live_channel()
>> >> +{
>> >> +   bool progress = false;
>> >> +   unsigned depth = 0;
>> >> +
>> >> +   foreach_block_and_inst_safe(block, fs_inst, inst, cfg) {
>> >> +      switch (inst->opcode) {
>> >> +      case BRW_OPCODE_IF:
>> >> +      case BRW_OPCODE_DO:
>> >> +         depth++;
>> >> +         break;
>> >> +
>> >> +      case BRW_OPCODE_ENDIF:
>> >> +      case BRW_OPCODE_WHILE:
>> >> +         depth--;
>> >> +         break;
>> >> +
>> >> +      case FS_OPCODE_DISCARD_JUMP:
>> >> +         /* This can potentially make control flow non-uniform until the end
>> >> +          * of the program.
>> >> +          */
>> >> +         depth++;
>> >> +         break;
>
> Why not just "return progress" at this point?  depth++ just makes it
> effectively never do anything again, but continue iterating over the
> rest of the program.
>
Heh, indeed.

> Otherwise, this all seems good to me.  This patch is
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
Thank you both for having a look. :)

>> >> +
>> >> +      case SHADER_OPCODE_FIND_LIVE_CHANNEL:
>> >> +         if (depth == 0) {
>> >> +            inst->opcode = BRW_OPCODE_MOV;
>> >> +            inst->src[0] = fs_reg(0);
>> >
>> > How does this work if we're on a primitive edge and channel zero is
>> > disabled? Does the EU not actually disable those channels except in
>> > the framebuffer write or something?
>> >
>> Yes, exactly, samples outside the primitive (or samples discarded by the
>> early depth/stencil test) have the corresponding execution mask bit
>> enabled, otherwise it wouldn't be possible to calculate derivatives near
>> the edge of a primitive.  Whole subspans not covered by the primitive or
>> dropped by early fragment tests are never dispatched to any PS thread,
>> so channel "0" should always have valid contents except when disabled by
>> control flow.
>> 
>> >> +            inst->sources = 1;
>> >> +            inst->force_writemask_all = true;
>> >> +            progress = true;
>> >> +         }
>> >> +         break;
>> >> +
>> >> +      default:
>> >> +         break;
>> >> +      }
>> >> +   }
>> >> +
>> >> +   return progress;
>> >> +}
>> >
>> > I wouldn't mind if this was just part of opt_algebraic. Less code that
>> > way and one fewer pass over the instruction list. What do you think,
>> > Ken?
>> 
>> I shortly considered doing that but finally decided not to because:
>>  - It's not an algebraic instruction.
>>  - The optimization is non-local unlike the other fully self-contained
>>    algebraic optimizations, as it needs to keep track of the control
>>    flow nesting level.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150504/f72bf7ac/attachment.sig>