[Mesa-dev] [PATCH 2/2] tgsi: Prevent emission of instructions with empty writemask.
Roland Scheidegger
sroland at vmware.com
Thu Nov 21 08:03:51 PST 2013
On 11/21/2013 02:01 PM, jfonseca at vmware.com wrote:
> From: José Fonseca <jfonseca at vmware.com>
>
> These degenerate instructions can often be emitted by state trackers
> when the semantics of instructions don't match precisely.
> ---
> src/gallium/auxiliary/tgsi/tgsi_ureg.c | 8 ++++++++
> src/gallium/auxiliary/tgsi/tgsi_ureg.h | 22 ++++++++++++++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_ureg.c b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> index 432ed00..f06858e 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
> @@ -1113,6 +1113,10 @@ ureg_insn(struct ureg_program *ureg,
> boolean negate = FALSE;
> unsigned swizzle[4] = { 0 };
>
> + if (nr_dst && ureg_dst_is_empty(dst[0])) {
> + return;
> + }
>
This is technically not really correct, as the instruction may have
multiple destinations.
Not saying we really have any such instructions (or even that we should
have them), but this helper (and the one below) has code to deal with
multiple destinations, so that's a bit inconsistent.
Also, there are instructions which have just one dst reg but side
effects, though it may be restricted to OPCODE_POPA (and I wouldn't mind
seeing it disappear) (I think the fence/atomic instructions might be
fine and at first glance I don't see anything else).
> saturate = nr_dst ? dst[0].Saturate : FALSE;
> predicate = nr_dst ? dst[0].Predicate : FALSE;
> if (predicate) {
> @@ -1162,6 +1166,10 @@ ureg_tex_insn(struct ureg_program *ureg,
> boolean negate = FALSE;
> unsigned swizzle[4] = { 0 };
>
> + if (nr_dst && ureg_dst_is_empty(dst[0])) {
> + return;
> + }
Same as above.
> saturate = nr_dst ? dst[0].Saturate : FALSE;
> predicate = nr_dst ? dst[0].Predicate : FALSE;
> if (predicate) {
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_ureg.h b/src/gallium/auxiliary/tgsi/tgsi_ureg.h
> index cf0c75e..d973edb 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_ureg.h
> +++ b/src/gallium/auxiliary/tgsi/tgsi_ureg.h
> @@ -454,6 +454,16 @@ ureg_imm1i( struct ureg_program *ureg,
> return ureg_DECL_immediate_int( ureg, &a, 1 );
> }
>
> +/* Where the destination register has a valid file, but an empty
> + * writemask.
> + */
> +static INLINE boolean
> +ureg_dst_is_empty( struct ureg_dst dst )
> +{
> + return dst.File != TGSI_FILE_NULL &&
> + dst.WriteMask == 0;
> +}
> +
> /***********************************************************************
> * Functions for patching up labels
> */
> @@ -650,6 +660,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -673,6 +684,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -697,6 +709,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -723,6 +736,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -750,6 +764,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> unsigned opcode = TGSI_OPCODE_##op; \
> unsigned target = TGSI_TEXTURE_UNKNOWN; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -777,6 +792,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -805,6 +821,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> unsigned opcode = TGSI_OPCODE_##op; \
> unsigned target = TGSI_TEXTURE_UNKNOWN; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -835,6 +852,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -866,6 +884,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> unsigned opcode = TGSI_OPCODE_##op; \
> unsigned target = TGSI_TEXTURE_UNKNOWN; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -897,6 +916,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -928,6 +948,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> { \
> unsigned opcode = TGSI_OPCODE_##op; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
> @@ -960,6 +981,7 @@ static INLINE void ureg_##op( struct ureg_program *ureg, \
> unsigned opcode = TGSI_OPCODE_##op; \
> unsigned target = TGSI_TEXTURE_UNKNOWN; \
> struct ureg_emit_insn_result insn; \
> + if (ureg_dst_is_empty(dst)) return; \
> insn = ureg_emit_insn(ureg, \
> opcode, \
> dst.Saturate, \
>
Otherwise this looks good to me, though I wouldn't insist it's really
necessary to prune such instructions.
Roland
More information about the mesa-dev
mailing list