On Tue, 2015-01-20 at 19:06 -0800, Anuj Phogat wrote: > On Mon, Jan 19, 2015 at 3:32 AM, Eduardo Lima Mitev <el...@igalia.com> wrote: > > From: Iago Toral Quiroga <ito...@igalia.com> > > > > For code such as: > > > > uint tmp1 = uint(in0); > > uint tmp2 = -tmp1; > > float out0 = float(tmp2); > > > > We produce code like: > > mov(8) g5<1>.xF -g9<4,4,1>.xUD > > > > which does not produce correct results. This code produces the > > results we would expect if tmp1 and tmp2 were signed integers > > instead. > > > > It seems that a similar problem was detected and addressed when > > using negations with unsigned integers as part of condionals, but > > it looks like the problem has a wider impact than that. > > > > This patch fixes the problem by preventing copy-propagation of > > negated UD registers in all scenarios, not only in conditionals. > > > > Fixes the following 8 dEQP tests: > > > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uint_vertex > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uint_fragment > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec2_vertex > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec2_fragment > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec3_vertex > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec3_fragment > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec4_vertex > > dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec4_fragment > > > > Note: > > For some reason the mediump and lowp versions of these tests still fail but > > I am not sure about the reason for that since the code we generate now seems > > correct (in fact, is the same as for the highp versions). These tests would > > need further investigation.
Thanks for testing and reviewing Anuj. I re-tested it too and realized this also fixes the mediump and lowp versions if the tests so I'll update the commit log accordingly. Iago > > --- > > src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 9 ++++++--- > > src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 9 ++++----- > > 2 files changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp > > b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp > > index 70f417f..5dd7255 100644 > > --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp > > @@ -302,9 +302,12 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, > > acp_entry *entry) > > (entry->dst.reg_offset + entry->regs_written) * 32) > > return false; > > > > - /* See resolve_ud_negate() and comment in brw_fs_emit.cpp. */ > > - if (inst->conditional_mod && > > - inst->src[arg].type == BRW_REGISTER_TYPE_UD && > > + /* we can't generally copy-propagate UD negations because we > > + * can end up accessing the resulting values as signed integers > > + * instead. See also resolve_ud_negate() and comment in > > + * fs_generator::generate_code. > > + */ > > + if (inst->src[arg].type == BRW_REGISTER_TYPE_UD && > > entry->src.negate) > > return false; > > > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > > b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > > index 9e47dd9..562ecb7 100644 > > --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp > > @@ -318,12 +318,11 @@ try_copy_propagate(struct brw_context *brw, > > vec4_instruction *inst, > > if (inst->is_send_from_grf()) > > return false; > > > > - /* We can't copy-propagate a UD negation into a condmod > > - * instruction, because the condmod ends up looking at the 33-bit > > - * signed accumulator value instead of the 32-bit value we wanted > > + /* we can't generally copy-propagate UD negations becuse we > > + * end up accessing the resulting values as signed integers > > + * instead. See also resolve_ud_negate(). > > */ > > - if (inst->conditional_mod && > > - value.negate && > > + if (value.negate && > > value.type == BRW_REGISTER_TYPE_UD) > > return false; > > > > -- > > 2.1.3 > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > Changes look good to me. Verified that patch now generates correct > code for the example in the comment. > > Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com> > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
