Re: [Mesa-dev] [PATCH 2/2] tgsi: Prevent emission of instructions with empty writemask.

Roland Scheidegger Fri, 22 Nov 2013 08:06:35 -0800

On 11/22/2013 03:03 PM, Jose Fonseca wrote:

Thanks for reviewing.


----- Original Message -----

On 11/21/2013 02:01 PM, jfons...@vmware.com wrote:

From: José Fonseca <jfons...@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

Oh right! Makes me wonder if we shouldn't get rid of multi-dst businesscompletely.



    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.

Ok thanks.

Roland


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

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] tgsi: Prevent emission of instructions with empty writemask.

Reply via email to