[Mesa-dev] [PATCH 2/3] i965: Drop the confusing saturate argument to math instruction setup.
Kenneth Graunke
kenneth at whitecape.org
Wed Aug 8 15:28:34 PDT 2012
On 08/08/2012 03:08 PM, Eric Anholt wrote:
> This was ridiculous. We were ignoring the inst->header.saturate flag in the
> case of math and only math. On gen4, we would leave inst->header.saturate in
> place if it happened to be set, which would end up being applied to the
> implicit mov and thus trash the first argument. On gen6, we would overwrite
> inst->header.saturate with the saturate flag from the argument, which was not
> set appropriately in brw_vec4_emit.cpp, and was only not a bug due to our
> incompetence at coalescing saturate moves.
>
> By ripping the argument out and making saturate work just like all the other
> brw_eu_emit.c code generation, we can avoid both these classes of bugs.
>
> Fixes piglit fog-modes, and the new specific fs-saturate-exp2 case.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=48628
> NOTE: This is a candidate for the 8.0 branch.
> ---
> src/mesa/drivers/dri/i965/brw_eu.h | 2 --
> src/mesa/drivers/dri/i965/brw_eu_emit.c | 14 ++++----------
> src/mesa/drivers/dri/i965/brw_eu_util.c | 1 -
> src/mesa/drivers/dri/i965/brw_fs_emit.cpp | 10 ----------
> src/mesa/drivers/dri/i965/brw_sf_emit.c | 2 --
> src/mesa/drivers/dri/i965/brw_vec4_emit.cpp | 3 ---
> src/mesa/drivers/dri/i965/brw_vs_emit.c | 3 ---
> src/mesa/drivers/dri/i965/brw_wm_emit.c | 15 ++-------------
> 8 files changed, 6 insertions(+), 44 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h b/src/mesa/drivers/dri/i965/brw_eu.h
> index 233b94c..6cab32d 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> @@ -975,7 +975,6 @@ void brw_SAMPLE(struct brw_compile *p,
> void brw_math_16( struct brw_compile *p,
> struct brw_reg dest,
> GLuint function,
> - GLuint saturate,
> GLuint msg_reg_nr,
> struct brw_reg src,
> GLuint precision );
> @@ -983,7 +982,6 @@ void brw_math_16( struct brw_compile *p,
> void brw_math( struct brw_compile *p,
> struct brw_reg dest,
> GLuint function,
> - GLuint saturate,
> GLuint msg_reg_nr,
> struct brw_reg src,
> GLuint data_type,
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> index 25bf91b..b82a858 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> @@ -429,7 +429,6 @@ static void brw_set_math_message( struct brw_compile *p,
> GLuint function,
> GLuint integer_type,
> bool low_precision,
> - bool saturate,
> GLuint dataType )
> {
> struct brw_context *brw = p->brw;
> @@ -461,22 +460,24 @@ static void brw_set_math_message( struct brw_compile *p,
> break;
> }
>
> +
> brw_set_message_descriptor(p, insn, BRW_SFID_MATH,
> msg_length, response_length, false, false);
> if (intel->gen == 5) {
> insn->bits3.math_gen5.function = function;
> insn->bits3.math_gen5.int_type = integer_type;
> insn->bits3.math_gen5.precision = low_precision;
> - insn->bits3.math_gen5.saturate = saturate;
> + insn->bits3.math_gen5.saturate = insn->header.saturate;
> insn->bits3.math_gen5.data_type = dataType;
> insn->bits3.math_gen5.snapshot = 0;
> } else {
> insn->bits3.math.function = function;
> insn->bits3.math.int_type = integer_type;
> insn->bits3.math.precision = low_precision;
> - insn->bits3.math.saturate = saturate;
> + insn->bits3.math.saturate = insn->header.saturate;
> insn->bits3.math.data_type = dataType;
> }
> + insn->header.saturate = 0;
> }
>
>
> @@ -1657,7 +1658,6 @@ void brw_WAIT (struct brw_compile *p)
> void brw_math( struct brw_compile *p,
> struct brw_reg dest,
> GLuint function,
> - GLuint saturate,
> GLuint msg_reg_nr,
> struct brw_reg src,
> GLuint data_type,
> @@ -1693,7 +1693,6 @@ void brw_math( struct brw_compile *p,
> * becomes FC[3:0] and ThreadCtrl becomes FC[5:4].
> */
> insn->header.destreg__conditionalmod = function;
> - insn->header.saturate = saturate;
>
> brw_set_dest(p, insn, dest);
> brw_set_src0(p, insn, src);
> @@ -1714,7 +1713,6 @@ void brw_math( struct brw_compile *p,
> function,
> src.type == BRW_REGISTER_TYPE_D,
> precision,
> - saturate,
> data_type);
> }
> }
> @@ -1779,7 +1777,6 @@ void brw_math2(struct brw_compile *p,
> void brw_math_16( struct brw_compile *p,
> struct brw_reg dest,
> GLuint function,
> - GLuint saturate,
> GLuint msg_reg_nr,
> struct brw_reg src,
> GLuint precision )
> @@ -1794,7 +1791,6 @@ void brw_math_16( struct brw_compile *p,
> * becomes FC[3:0] and ThreadCtrl becomes FC[5:4].
> */
> insn->header.destreg__conditionalmod = function;
> - insn->header.saturate = saturate;
>
> /* Source modifiers are ignored for extended math instructions. */
> assert(!src.negate);
> @@ -1822,7 +1818,6 @@ void brw_math_16( struct brw_compile *p,
> function,
> BRW_MATH_INTEGER_UNSIGNED,
> precision,
> - saturate,
> BRW_MATH_DATA_VECTOR);
>
> /* Second instruction:
> @@ -1838,7 +1833,6 @@ void brw_math_16( struct brw_compile *p,
> function,
> BRW_MATH_INTEGER_UNSIGNED,
> precision,
> - saturate,
> BRW_MATH_DATA_VECTOR);
>
> brw_pop_insn_state(p);
> diff --git a/src/mesa/drivers/dri/i965/brw_eu_util.c b/src/mesa/drivers/dri/i965/brw_eu_util.c
> index 5405cf1..2037634 100644
> --- a/src/mesa/drivers/dri/i965/brw_eu_util.c
> +++ b/src/mesa/drivers/dri/i965/brw_eu_util.c
> @@ -42,7 +42,6 @@ void brw_math_invert( struct brw_compile *p,
> brw_math( p,
> dst,
> BRW_MATH_FUNCTION_INV,
> - BRW_MATH_SATURATE_NONE,
> 0,
> src,
> BRW_MATH_PRECISION_FULL,
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> index a5cebcb..4564e3b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> @@ -160,8 +160,6 @@ fs_visitor::generate_math1_gen7(fs_inst *inst,
> assert(inst->mlen == 0);
> brw_math(p, dst,
> brw_math_function(inst->opcode),
> - inst->saturate ? BRW_MATH_SATURATE_SATURATE
> - : BRW_MATH_SATURATE_NONE,
> 0, src0,
> BRW_MATH_DATA_VECTOR,
> BRW_MATH_PRECISION_FULL);
> @@ -189,8 +187,6 @@ fs_visitor::generate_math1_gen6(fs_inst *inst,
> brw_set_compression_control(p, BRW_COMPRESSION_NONE);
> brw_math(p, dst,
> op,
> - inst->saturate ? BRW_MATH_SATURATE_SATURATE :
> - BRW_MATH_SATURATE_NONE,
> 0, src0,
> BRW_MATH_DATA_VECTOR,
> BRW_MATH_PRECISION_FULL);
> @@ -199,8 +195,6 @@ fs_visitor::generate_math1_gen6(fs_inst *inst,
> brw_set_compression_control(p, BRW_COMPRESSION_2NDHALF);
> brw_math(p, sechalf(dst),
> op,
> - inst->saturate ? BRW_MATH_SATURATE_SATURATE :
> - BRW_MATH_SATURATE_NONE,
> 0, sechalf(src0),
> BRW_MATH_DATA_VECTOR,
> BRW_MATH_PRECISION_FULL);
> @@ -240,8 +234,6 @@ fs_visitor::generate_math_gen4(fs_inst *inst,
> brw_set_compression_control(p, BRW_COMPRESSION_NONE);
> brw_math(p, dst,
> op,
> - inst->saturate ? BRW_MATH_SATURATE_SATURATE :
> - BRW_MATH_SATURATE_NONE,
> inst->base_mrf, src,
> BRW_MATH_DATA_VECTOR,
> BRW_MATH_PRECISION_FULL);
> @@ -250,8 +242,6 @@ fs_visitor::generate_math_gen4(fs_inst *inst,
> brw_set_compression_control(p, BRW_COMPRESSION_2NDHALF);
> brw_math(p, sechalf(dst),
> op,
> - inst->saturate ? BRW_MATH_SATURATE_SATURATE :
> - BRW_MATH_SATURATE_NONE,
> inst->base_mrf + 1, sechalf(src),
> BRW_MATH_DATA_VECTOR,
> BRW_MATH_PRECISION_FULL);
> diff --git a/src/mesa/drivers/dri/i965/brw_sf_emit.c b/src/mesa/drivers/dri/i965/brw_sf_emit.c
> index ff6383b..5f3673b 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_sf_emit.c
> @@ -314,7 +314,6 @@ static void invert_det( struct brw_sf_compile *c)
> brw_math(&c->func,
> c->inv_det,
> BRW_MATH_FUNCTION_INV,
> - BRW_MATH_SATURATE_NONE,
> 0,
> c->det,
> BRW_MATH_DATA_SCALAR,
> @@ -599,7 +598,6 @@ void brw_emit_point_sprite_setup(struct brw_sf_compile *c, bool allocate)
> brw_math(&c->func,
> c->tmp,
> BRW_MATH_FUNCTION_INV,
> - BRW_MATH_SATURATE_NONE,
> 0,
> c->dx0,
> BRW_MATH_DATA_SCALAR,
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp b/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp
> index 7658bb8..21eafcb 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_emit.cpp
> @@ -262,7 +262,6 @@ vec4_visitor::generate_math1_gen4(vec4_instruction *inst,
> brw_math(p,
> dst,
> brw_math_function(inst->opcode),
> - BRW_MATH_SATURATE_NONE,
> inst->base_mrf,
> src,
> BRW_MATH_DATA_VECTOR,
> @@ -291,7 +290,6 @@ vec4_visitor::generate_math1_gen6(vec4_instruction *inst,
> brw_math(p,
> dst,
> brw_math_function(inst->opcode),
> - BRW_MATH_SATURATE_NONE,
> inst->base_mrf,
> src,
> BRW_MATH_DATA_SCALAR,
> @@ -355,7 +353,6 @@ vec4_visitor::generate_math2_gen4(vec4_instruction *inst,
> brw_math(p,
> dst,
> brw_math_function(inst->opcode),
> - BRW_MATH_SATURATE_NONE,
> inst->base_mrf,
> op0,
> BRW_MATH_DATA_VECTOR,
> diff --git a/src/mesa/drivers/dri/i965/brw_vs_emit.c b/src/mesa/drivers/dri/i965/brw_vs_emit.c
> index d0ee5f3..6169f73 100644
> --- a/src/mesa/drivers/dri/i965/brw_vs_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_vs_emit.c
> @@ -670,7 +670,6 @@ static void emit_math1_gen4(struct brw_vs_compile *c,
> brw_math(p,
> tmp,
> function,
> - BRW_MATH_SATURATE_NONE,
> 2,
> arg0,
> BRW_MATH_DATA_SCALAR,
> @@ -705,7 +704,6 @@ emit_math1_gen6(struct brw_vs_compile *c,
> brw_math(p,
> tmp_dst,
> function,
> - BRW_MATH_SATURATE_NONE,
> 2,
> tmp_src,
> BRW_MATH_DATA_SCALAR,
> @@ -757,7 +755,6 @@ static void emit_math2_gen4( struct brw_vs_compile *c,
> brw_math(p,
> tmp,
> function,
> - BRW_MATH_SATURATE_NONE,
> 2,
> arg0,
> BRW_MATH_DATA_SCALAR,
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_emit.c b/src/mesa/drivers/dri/i965/brw_wm_emit.c
> index 61f66e7..b6defa3 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_emit.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_emit.c
> @@ -297,13 +297,11 @@ void emit_pixel_w(struct brw_wm_compile *c,
> if (c->dispatch_width == 16) {
> brw_math_16(p, dst[3],
> BRW_MATH_FUNCTION_INV,
> - BRW_MATH_SATURATE_NONE,
> 2, src,
> BRW_MATH_PRECISION_FULL);
> } else {
> brw_math(p, dst[3],
> BRW_MATH_FUNCTION_INV,
> - BRW_MATH_SATURATE_NONE,
> 2, src,
> BRW_MATH_DATA_VECTOR,
> BRW_MATH_PRECISION_FULL);
> @@ -890,9 +888,6 @@ void emit_math1(struct brw_wm_compile *c,
> struct brw_compile *p = &c->func;
> struct intel_context *intel = &p->brw->intel;
> int dst_chan = ffs(mask & WRITEMASK_XYZW) - 1;
> - GLuint saturate = ((mask & SATURATE) ?
> - BRW_MATH_SATURATE_SATURATE :
> - BRW_MATH_SATURATE_NONE);
> struct brw_reg src;
>
> if (!(mask & WRITEMASK_XYZW))
> @@ -918,11 +913,11 @@ void emit_math1(struct brw_wm_compile *c,
> /* Send two messages to perform all 16 operations:
> */
> brw_push_insn_state(p);
> + brw_set_saturate(p, (mask & SATURATE) ? 1 : 0);
With my stdbool suggestion, you could drop the silly ? 1 : 0.
> brw_set_compression_control(p, BRW_COMPRESSION_NONE);
> brw_math(p,
> dst[dst_chan],
> function,
> - saturate,
> 2,
> src,
> BRW_MATH_DATA_VECTOR,
> @@ -933,7 +928,6 @@ void emit_math1(struct brw_wm_compile *c,
> brw_math(p,
> offset(dst[dst_chan],1),
> function,
> - saturate,
> 3,
> sechalf(src),
> BRW_MATH_DATA_VECTOR,
> @@ -1005,10 +999,6 @@ void emit_math2(struct brw_wm_compile *c,
> sechalf(src1));
> }
> } else {
> - GLuint saturate = ((mask & SATURATE) ?
> - BRW_MATH_SATURATE_SATURATE :
> - BRW_MATH_SATURATE_NONE);
> -
> brw_set_compression_control(p, BRW_COMPRESSION_NONE);
> brw_MOV(p, brw_message_reg(3), arg1[0]);
> if (c->dispatch_width == 16) {
> @@ -1016,11 +1006,11 @@ void emit_math2(struct brw_wm_compile *c,
> brw_MOV(p, brw_message_reg(5), sechalf(arg1[0]));
> }
>
> + brw_set_saturate(p, (mask & SATURATE) ? 1 : 0);
Likewise, stdbool saves you from ? 1 : 0.
> brw_set_compression_control(p, BRW_COMPRESSION_NONE);
> brw_math(p,
> dst[dst_chan],
> function,
> - saturate,
> 2,
> arg0[0],
> BRW_MATH_DATA_VECTOR,
> @@ -1033,7 +1023,6 @@ void emit_math2(struct brw_wm_compile *c,
> brw_math(p,
> offset(dst[dst_chan],1),
> function,
> - saturate,
> 4,
> sechalf(arg0[0]),
> BRW_MATH_DATA_VECTOR,
>
This looks great, Eric.
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
More information about the mesa-dev
mailing list