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-sta...@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);
> +}
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
