[Mesa-dev] [Mesa-stable] [PATCH 2/3] i965/fs: Consider cmod when propagating to inst with different type.

[Ian Romanick]

On 02/27/2015 11:34 AM, Matt Turner wrote:
> We can safely propagate the conditional mod to an instruction with a
> different type if the conditional mod does not involve comparing for
> equality with zero (or probably NaN, but ignore that for now).
> 
> This is because -0.0 and +0.0 are both test equal to zero, but their
> integer representations do not.

That sounds really sketchy.  Did you try to make a stand-alone test to
reproduce the problem based on your theory?

> Cc: 10.5 <mesa-stable at lists.freedesktop.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89317
> ---
>  .../drivers/dri/i965/brw_fs_cmod_propagation.cpp   |  8 +++
>  .../drivers/dri/i965/test_fs_cmod_propagation.cpp  | 67 ++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp
> index c6384ab..a92eef6 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp
> @@ -80,6 +80,14 @@ opt_cmod_propagation_local(fs_visitor *v, bblock_t *block)
>                  scan_inst->dst.reg_offset != inst->src[0].reg_offset)
>                 break;
>  
> +            /* Testing equality with zero is different for ints and floats */
> +            if (scan_inst->dst.type != inst->dst.type &&
> +                (inst->conditional_mod == BRW_CONDITIONAL_Z ||
> +                 inst->conditional_mod == BRW_CONDITIONAL_NZ ||
> +                 inst->conditional_mod == BRW_CONDITIONAL_GE ||
> +                 inst->conditional_mod == BRW_CONDITIONAL_LE))
> +               break;
> +
>              /* If the instruction generating inst's source also wrote the
>               * flag, and inst is doing a simple .nz comparison, then inst
>               * is redundant - the appropriate value is already in the flag
> diff --git a/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp b/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp
> index fbe4fd9..0287161 100644
> --- a/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp
> @@ -415,3 +415,70 @@ TEST_F(cmod_propagation_test, movnz)
>     EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 0)->opcode);
>     EXPECT_EQ(BRW_CONDITIONAL_GE, instruction(block0, 0)->conditional_mod);
>  }
> +
> +TEST_F(cmod_propagation_test, different_types)
> +{
> +   fs_reg dest = v->vgrf(glsl_type::int_type);
> +   fs_reg src0 = v->vgrf(glsl_type::int_type);
> +   fs_reg src1 = v->vgrf(glsl_type::int_type);
> +   fs_reg zero(0.0f);
> +   v->emit(BRW_OPCODE_ADD, dest, src0, src1);
> +   v->emit(BRW_OPCODE_CMP, v->reg_null_f, retype(dest, BRW_REGISTER_TYPE_F),
> +                                          zero)
> +      ->conditional_mod = BRW_CONDITIONAL_G;
> +
> +   /* = Before =
> +    *
> +    * 0: add(8)        dest:D  src0:D  src1:D
> +    * 1: cmp.g.f0(8)   null:F  dest:F  0.0f
> +    *
> +    * = After =
> +    * 0: add.g.f0(8)   dest:D  src0:D  src1:D
> +    */
> +
> +   v->calculate_cfg();
> +   bblock_t *block0 = v->cfg->blocks[0];
> +
> +   EXPECT_EQ(0, block0->start_ip);
> +   EXPECT_EQ(1, block0->end_ip);
> +
> +   EXPECT_TRUE(cmod_propagation(v));
> +   EXPECT_EQ(0, block0->start_ip);
> +   EXPECT_EQ(0, block0->end_ip);
> +   EXPECT_EQ(BRW_OPCODE_ADD, instruction(block0, 0)->opcode);
> +   EXPECT_EQ(BRW_CONDITIONAL_G, instruction(block0, 0)->conditional_mod);
> +}
> +
> +TEST_F(cmod_propagation_test, different_types_cmod_with_zero)
> +{
> +   fs_reg dest = v->vgrf(glsl_type::int_type);
> +   fs_reg src0 = v->vgrf(glsl_type::int_type);
> +   fs_reg src1 = v->vgrf(glsl_type::int_type);
> +   fs_reg zero(0.0f);
> +   v->emit(BRW_OPCODE_ADD, dest, src0, src1);
> +   v->emit(BRW_OPCODE_CMP, v->reg_null_f, retype(dest, BRW_REGISTER_TYPE_F),
> +                                          zero)
> +      ->conditional_mod = BRW_CONDITIONAL_GE;
> +
> +   /* = Before =
> +    *
> +    * 0: add(8)        dest:D  src0:D  src1:D
> +    * 1: cmp.ge.f0(8)  null:F  dest:F  0.0f
> +    *
> +    * = After =
> +    * (no changes)
> +    */
> +
> +   v->calculate_cfg();
> +   bblock_t *block0 = v->cfg->blocks[0];
> +
> +   EXPECT_EQ(0, block0->start_ip);
> +   EXPECT_EQ(1, block0->end_ip);
> +
> +   EXPECT_FALSE(cmod_propagation(v));
> +   EXPECT_EQ(0, block0->start_ip);
> +   EXPECT_EQ(1, block0->end_ip);
> +   EXPECT_EQ(BRW_OPCODE_ADD, instruction(block0, 0)->opcode);
> +   EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 1)->opcode);
> +   EXPECT_EQ(BRW_CONDITIONAL_GE, instruction(block0, 1)->conditional_mod);
> +}
>