[Mesa-dev] [PATCH 2/2] tgsi: Prevent emission of instructions with empty writemask.
Jose Fonseca
jfonseca at vmware.com
Fri Nov 22 07:03:20 PST 2013
Thanks for reviewing.
----- Original Message -----
> 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.
The assumption there is only one dst is already there. See the code immediately below
saturate = nr_dst ? dst[0].Saturate : FALSE;
predicate = nr_dst ? dst[0].Predicate : FALSE;
I rather commit this as is now, as the multiple destination is not a real problem ATM, while the empty writemask do seem to arise due to me earlier commit.
> 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).
POPA is not used anywhere and can be removed. I'll do that in another commit.
Jose
>
> > 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