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

[Kenneth Graunke]

On Friday, February 27, 2015 11:34:06 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.
> 
> 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
> +    */

This is wrong - you're adding two integers and doing an integer compare
with 0, rather than a float comparison.

I verified that in C, (-0.0 > 0.0) == false.  I then modified your
Piglit test to be:

        if (uintBitsToFloat(a + b) > 0.0)
                frag_color = vec4(1.0, 0.0, 0.0, 1.0);
        else
                frag_color = vec4(0.0, 1.0, 0.0, 1.0);

(> instead of ==, swap red/green).

This fails with your code, because it ends up doing:
      0x80000000 > 0x0
   => 0x80000000 - 0x0 > 0x0.
   => 0x80000000 > 0
   => True

It passes if you eliminate the conditional_mod checks and just do:

   if (scan_inst->dst.type != inst->dst.type)
      break;

Assuming you make that change, and drop/fix this unit test, you can put
my R-b on it.  Good find :)

> +
> +   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);
> +}
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150303/6bcbf65b/attachment.sig>