[Mesa-dev] [PATCH 1/2] i965/fs: Optimize conditional discards.

[Matt Turner]

On Mon, Aug 18, 2014 at 5:50 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Almost all uses of discard in GLSL take the form:
>
>     if (some_condition)
>         discard;
>
> This generates the following assembly:
>
>     cmp.condition.f0.0 src0 src1 /* generate some condition */
>     IF
>        (+f0.1) cmp.ne.f0.1 g0 g0 /* figure out enabled channels */
>        (-f0.1.anyNh) halt        /* jump to end of program */
>     ENDIF
>
> We can optimize this pretty easily.  The original CMP already sets the
> flag register to the channels we want to discard, so we can simply use
> that instead of disabling channels we want to keep, then checking which
> channels are still running.  This works out to:
>
>     (+f0.1) CMP.negated_condition.f0.1 src0 src1
>     (-f0.1.anyNh) halt
>
> In addition to removing three instructions, this combines basic blocks,
> which can lead to more optimization opportunities.
>
> total instructions in shared programs: 5135997 -> 5122144 (-0.27%)
> instructions in affected programs:     313174 -> 299321 (-4.42%)
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_eu.c   | 22 ++++++++++++++
>  src/mesa/drivers/dri/i965/brw_eu.h   |  1 +
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 57 ++++++++++++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_fs.h   |  1 +
>  4 files changed, 81 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.c b/src/mesa/drivers/dri/i965/brw_eu.c
> index 9905972..a1b7fda 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu.c
> @@ -65,6 +65,28 @@ brw_reg_type_letters(unsigned type)
>     return names[type];
>  }
>
> +/* Returns a conditional modifier that negates the condition. */
> +enum brw_conditional_mod
> +brw_negate_cmod(uint32_t cmod)
> +{
> +   switch (cmod) {
> +   case BRW_CONDITIONAL_Z:
> +      return BRW_CONDITIONAL_NZ;
> +   case BRW_CONDITIONAL_NZ:
> +      return BRW_CONDITIONAL_Z;
> +   case BRW_CONDITIONAL_G:
> +      return BRW_CONDITIONAL_LE;
> +   case BRW_CONDITIONAL_GE:
> +      return BRW_CONDITIONAL_L;
> +   case BRW_CONDITIONAL_L:
> +      return BRW_CONDITIONAL_GE;
> +   case BRW_CONDITIONAL_LE:
> +      return BRW_CONDITIONAL_G;
> +   default:
> +      return ~0;

BRW_CONDITIONAL_NONE?

> +   }
> +}
> +
>  /* Returns the corresponding conditional mod for swapping src0 and
>   * src1 in e.g. CMP.
>   */
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h b/src/mesa/drivers/dri/i965/brw_eu.h
> index e6c26e3..c6cc239 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -447,6 +447,7 @@ void brw_set_src1(struct brw_compile *p, brw_inst *insn, struct brw_reg reg);
>
>  void brw_set_uip_jip(struct brw_compile *p);
>
> +enum brw_conditional_mod brw_negate_cmod(uint32_t cmod);
>  enum brw_conditional_mod brw_swap_cmod(uint32_t cmod);
>
>  /* brw_eu_compact.c */
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index f1d3fb8..6d80603 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -2122,6 +2122,62 @@ fs_visitor::opt_register_renaming()
>  }
>
>  bool
> +fs_visitor::opt_conditional_discard()
> +{
> +   bool progress = false;
> +
> +   foreach_in_list(fs_inst, inst, &instructions) {

I'd be nice to rebase this code on my save-cfg series and remove
things from blocks.

Doing this when you're actually deleting control flow is kind of
annoying (see i965: Preserve CFG when deleting dead control flow.)
though. I'd probably do it in such a way as to leave the if and endif
instructions in place, and just pull the discard instructions out of
the control flow, and then let dead-control-flow-eliminate do the
annoying work.

Then again, wouldn't it be easier to recognize if (...) discard in the
visitor? I'd be willing to bet that nearly everything recognized by
this would be recognized by a visitor peephole, given that we don't
have optimizations to pull things out of control flow.

What do you think about trying this in the visitor?

> +      fs_inst *iff, *discard, *jump, *endif;
> +
> +      /* Match the following pattern:
> +       *    CMP                      (inst)
> +       *    IF                       (iff)
> +       *    (+f0.1) cmp.ne.f0.1      (discard)
> +       *    (-f0.1.anyNh) halt       (jump)
> +       *    ENDIF                    (endif)
> +       */
> +      bool match =
> +         inst->opcode == BRW_OPCODE_CMP &&
> +         (iff = (fs_inst *) inst->next)->opcode == BRW_OPCODE_IF &&
> +         (discard = (fs_inst *) iff->next)->opcode == BRW_OPCODE_CMP &&
> +         discard->predicate == BRW_PREDICATE_NORMAL &&
> +         discard->flag_subreg == 1 &&
> +         (jump = (fs_inst *) discard->next)->opcode == FS_OPCODE_DISCARD_JUMP &&
> +         jump->flag_subreg == 1 &&
> +         (endif = (fs_inst *) jump->next)->opcode == BRW_OPCODE_ENDIF;
> +
> +      if (!match)
> +         continue;
> +
> +      progress = true;
> +
> +      /* Rewrite the discard CMP to use the condition for the IF, only
> +       * negated (since we need a false comparison, i.e. g0 != g0).
> +       */
> +      discard->src[0] = inst->src[0];
> +      discard->src[1] = inst->src[1];
> +      discard->conditional_mod = brw_negate_cmod(inst->conditional_mod);
> +      iff->remove();
> +      endif->remove();
> +
> +      /* TODO: We should let dead code elimination remove the original CMP.

I don't really think so. It's easy enough to remove it here, and
you've already done the analysis to do so, so you might as well do it.
I'd drop the whole comment. I guess theoretically something after the
control flow could use the flag result.

> +       * It currently is not smart enough to handle that.  However, we know
> +       * that CSE can't have reused the value of f0 for anything else, since
> +       * it works on basic blocks, and this CMP is at the end of a basic
> +       * block (since it was immediately followed by an IF).  So it should
> +       * be safe for us to remove it.
> +       */
> +      inst->remove();
> +      inst = jump;