Re: [Mesa-dev] [PATCH 03/11] i965: Fix negate with unsigned integers
On Tue, 2015-01-20 at 19:06 -0800, Anuj Phogat wrote: > On Mon, Jan 19, 2015 at 3:32 AM, Eduardo Lima Mitev wrote: > > From: Iago Toral Quiroga > > > > 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 > ___ > 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
Re: [Mesa-dev] [PATCH 03/11] i965: Fix negate with unsigned integers
On Mon, Jan 19, 2015 at 3:32 AM, Eduardo Lima Mitev wrote: > From: Iago Toral Quiroga > > 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. > --- > 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 03/11] i965: Fix negate with unsigned integers
From: Iago Toral Quiroga 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. --- 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
