[Mesa-dev] [PATCH 1/2] i965: Do not apply CSE opt to MOV immediate

[Matt Turner]

On Wed, Nov 25, 2015 at 5:36 AM, Juan A. Suarez Romero
<jasuarez at igalia.com> wrote:
> If the shader asm code is something like:
>
> mov vgrf2767.0:F, [13F, 14F, 15F, 16F]
> mov vgrf2768.0:F, [9F, 10F, 11F, 12F]
> mov m8:F, [13F, 14F, 15F, 16F]
> mov m7:F, [9F, 10F, 11F, 12F]
>
> And we apply Common Subexpresion Elimination optimization, we get the
> following one:
>
> mov vgrf2771.0:F, [13F, 14F, 15F, 16F]
> mov vgrf2767.0:F, vgrf2771.xyzw:F
> mov vgrf2772.0:F, [9F, 10F, 11F, 12F]
> mov vgrf2768.0:F, vgrf2772.xyzw:F
> mov m8:F, vgrf2771.xyzw:F
> mov m7:F, vgrf2772.xyzw:F
>
> The problem is that later we apply Copy Propagation optimization, which
> reverts the change to the original one. If we run the optimizations in
> a loop, there is always a progress, but we are in neverending loop.
>
> Usually, when we have a sentence of the form:
>
> X = exp
>
> We apply CSE if "exp" is actually an expression. But if it is constant
> we do not apply it, as the point of CSE is saving running the same
> computation more than once, that doesn't happen when we have a constant.
>
> So this commit ensures CSE is not applied to MOV immediate (as it
> provides no gain, and it is reverted later by copy-propagation
> optimization).
>
> Signed-off-by: Juan A. Suarez Romero <jasuarez at igalia.com>
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_cse.cpp | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp
> index 85cbf24..7ed7654 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp
> @@ -93,6 +93,17 @@ is_expression(const vec4_instruction *const inst)
>  }
>
>  static bool
> +is_mov_constant(const vec4_instruction *const inst)
> +{
> +   if (inst->opcode == BRW_OPCODE_MOV &&
> +       inst->src[0].file == BRW_IMMEDIATE_VALUE) {
> +      return true;
> +   } else {
> +      return false;
> +   }
> +}
> +
> +static bool
>  operands_match(const vec4_instruction *a, const vec4_instruction *b)
>  {
>     const src_reg *xs = a->src;
> @@ -142,7 +153,7 @@ vec4_visitor::opt_cse_local(bblock_t *block)
>     int ip = block->start_ip;
>     foreach_inst_in_block (vec4_instruction, inst, block) {
>        /* Skip some cases. */
> -      if (is_expression(inst) && !inst->predicate && inst->mlen == 0 &&
> +      if (is_expression(inst) && !inst->predicate && inst->mlen == 0 && !is_mov_constant(inst) &&
>            ((inst->dst.file != ARF && inst->dst.file != FIXED_GRF) ||
>             inst->dst.is_null()))
>        {
> --

I'm sending a number of comments on PATCH 2/2 which might make this
patch unnecessary, but in case it is necessary, I think you can
simplify it by adding

      return inst->src[0].file != BRW_IMMEDIATE_VALUE;

immediately after case BRW_OPCODE_MOV: in is_expression(). That's at
least what I did when I tried something similar before when adding
opt_vector_float().