i965/vec4/dce: Don't narrow the write mask if the flags are used

In an instruction sequence like

            cmp(8).ge.f0.0 vgrf17:D, vgrf2.xxxx:D, vgrf9.xxxx:D
    (+f0.0) sel(8) vgrf1:UD, vgrf8.xyzw:UD, vgrf1.xyzw:UD

The other fields of vgrf17 may be unused, but the CMP still needs to
generate the other flag bits.

To my surprise, nothing in shader-db or any test suite appears to hit
this.  However, I have a change to brw_vec4_cmod_propagation that
creates cases where this can happen.  This fix prevents a couple dozen
regressions in that patch.

Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: 5df88c20 ("i965/vec4: Rewrite dead code elimination to use live in/out.")

diff --git a/src/intel/Makefile.compiler.am b/src/intel/Makefile.compiler.am
index cd7e6882fb9f0c95fae316048f7bb42bef9e4fdf..7c33e35816b01ba22e7cdd6aa4a09fbf8d5d6ad8 100644 (file)
--- a/src/intel/Makefile.compiler.am
+++ b/src/intel/Makefile.compiler.am
@@ -64,6 +64,7 @@ COMPILER_TESTS = \
        compiler/test_vf_float_conversions \
        compiler/test_vec4_cmod_propagation \
        compiler/test_vec4_copy_propagation \
+       compiler/test_vec4_dead_code_eliminate \
        compiler/test_vec4_register_coalesce
 
 TESTS += $(COMPILER_TESTS)
@@ -97,6 +98,10 @@ compiler_test_vec4_cmod_propagation_SOURCES = \
        compiler/test_vec4_cmod_propagation.cpp
 compiler_test_vec4_cmod_propagation_LDADD = $(TEST_LIBS)
 
+compiler_test_vec4_dead_code_eliminate_SOURCES = \
+       compiler/test_vec4_dead_code_eliminate.cpp
+compiler_test_vec4_dead_code_eliminate_LDADD = $(TEST_LIBS)
+
 # Strictly speaking this is neither a C++ test nor using gtest - we can address
 # address that at a later point. Until then, this allows us a to simplify things.
 compiler_test_eu_compact_SOURCES = \

diff --git a/src/intel/compiler/brw_vec4_dead_code_eliminate.cpp b/src/intel/compiler/brw_vec4_dead_code_eliminate.cpp
index c09a3d7ebe919e8d763e91e3715a24496b413635..99e4c9cacaf3bea1a68f1263a026c9f7341da908 100644 (file)
--- a/src/intel/compiler/brw_vec4_dead_code_eliminate.cpp
+++ b/src/intel/compiler/brw_vec4_dead_code_eliminate.cpp
@@ -81,17 +81,46 @@ vec4_visitor::dead_code_eliminate()
                result_live[3] = result;
             }
 
-            for (int c = 0; c < 4; c++) {
-               if (!result_live[c] && inst->dst.writemask & (1 << c)) {
-                  inst->dst.writemask &= ~(1 << c);
+            if (inst->writes_flag()) {
+               /* Independently calculate the usage of the flag components and
+                * the destination value components.
+                */
+               uint8_t flag_mask = inst->dst.writemask;
+               uint8_t dest_mask = inst->dst.writemask;
+
+               for (int c = 0; c < 4; c++) {
+                  if (!result_live[c] && dest_mask & (1 << c))
+                     dest_mask &= ~(1 << c);
+
+                  if (!BITSET_TEST(flag_live, c))
+                     flag_mask &= ~(1 << c);
+               }
+
+               if (inst->dst.writemask != (flag_mask | dest_mask)) {
                   progress = true;
+                  inst->dst.writemask = flag_mask | dest_mask;
+               }
 
-                  if (inst->dst.writemask == 0) {
-                     if (inst->writes_accumulator || inst->writes_flag()) {
-                        inst->dst = dst_reg(retype(brw_null_reg(), inst->dst.type));
-                     } else {
-                        inst->opcode = BRW_OPCODE_NOP;
-                        break;
+               /* If none of the destination components are read, replace the
+                * destination register with the NULL register.
+                */
+               if (dest_mask == 0) {
+                  progress = true;
+                  inst->dst = dst_reg(retype(brw_null_reg(), inst->dst.type));
+               }
+            } else {
+               for (int c = 0; c < 4; c++) {
+                  if (!result_live[c] && inst->dst.writemask & (1 << c)) {
+                     inst->dst.writemask &= ~(1 << c);
+                     progress = true;
+
+                     if (inst->dst.writemask == 0) {
+                        if (inst->writes_accumulator) {
+                           inst->dst = dst_reg(retype(brw_null_reg(), inst->dst.type));
+                        } else {
+                           inst->opcode = BRW_OPCODE_NOP;
+                           break;
+                        }
                      }
                   }
                }

diff --git a/src/intel/compiler/meson.build b/src/intel/compiler/meson.build
index 2124278cc04f8d45839381b525b0b481f434bd81..69ce2eab4cf887a6648b635e773ed84ef22f98f8 100644 (file)
--- a/src/intel/compiler/meson.build
+++ b/src/intel/compiler/meson.build
@@ -146,7 +146,8 @@ if with_tests
   foreach t : ['fs_cmod_propagation', 'fs_copy_propagation',
                'fs_saturate_propagation', 'vf_float_conversions',
                'vec4_register_coalesce', 'vec4_copy_propagation',
-               'vec4_cmod_propagation', 'eu_compact', 'eu_validate']
+               'vec4_cmod_propagation', 'vec4_dead_code_eliminate',
+               'eu_compact', 'eu_validate']
     test(
       t,
       executable(

diff --git a/src/intel/compiler/test_vec4_dead_code_eliminate.cpp b/src/intel/compiler/test_vec4_dead_code_eliminate.cpp
new file mode 100644 (file)
index 0000000..25739c2
--- /dev/null
+++ b/src/intel/compiler/test_vec4_dead_code_eliminate.cpp
@@ -0,0 +1,163 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <gtest/gtest.h>
+#include "brw_vec4.h"
+#include "program/program.h"
+
+using namespace brw;
+
+class dead_code_eliminate_test : public ::testing::Test {
+   virtual void SetUp();
+
+public:
+   struct brw_compiler *compiler;
+   struct gen_device_info *devinfo;
+   struct gl_context *ctx;
+   struct gl_shader_program *shader_prog;
+   struct brw_vue_prog_data *prog_data;
+   vec4_visitor *v;
+};
+
+class dead_code_eliminate_vec4_visitor : public vec4_visitor
+{
+public:
+   dead_code_eliminate_vec4_visitor(struct brw_compiler *compiler,
+                                 nir_shader *shader,
+                                 struct brw_vue_prog_data *prog_data)
+      : vec4_visitor(compiler, NULL, NULL, prog_data, shader, NULL,
+                     false /* no_spills */, -1)
+   {
+      prog_data->dispatch_mode = DISPATCH_MODE_4X2_DUAL_OBJECT;
+   }
+
+protected:
+   virtual dst_reg *make_reg_for_system_value(int /* location */)
+   {
+      unreachable("Not reached");
+   }
+
+   virtual void setup_payload()
+   {
+      unreachable("Not reached");
+   }
+
+   virtual void emit_prolog()
+   {
+      unreachable("Not reached");
+   }
+
+   virtual void emit_thread_end()
+   {
+      unreachable("Not reached");
+   }
+
+   virtual void emit_urb_write_header(int /* mrf */)
+   {
+      unreachable("Not reached");
+   }
+
+   virtual vec4_instruction *emit_urb_write_opcode(bool /* complete */)
+   {
+      unreachable("Not reached");
+   }
+};
+
+
+void dead_code_eliminate_test::SetUp()
+{
+   ctx = (struct gl_context *)calloc(1, sizeof(*ctx));
+   compiler = (struct brw_compiler *)calloc(1, sizeof(*compiler));
+   devinfo = (struct gen_device_info *)calloc(1, sizeof(*devinfo));
+   prog_data = (struct brw_vue_prog_data *)calloc(1, sizeof(*prog_data));
+   compiler->devinfo = devinfo;
+
+   nir_shader *shader =
+      nir_shader_create(NULL, MESA_SHADER_VERTEX, NULL, NULL);
+
+   v = new dead_code_eliminate_vec4_visitor(compiler, shader, prog_data);
+
+   devinfo->gen = 4;
+}
+
+static void
+dead_code_eliminate(vec4_visitor *v)
+{
+   bool print = false;
+
+   if (print) {
+      fprintf(stderr, "instructions before:\n");
+      v->dump_instructions();
+   }
+
+   v->calculate_cfg();
+   v->dead_code_eliminate();
+
+   if (print) {
+      fprintf(stderr, "instructions after:\n");
+      v->dump_instructions();
+   }
+}
+
+TEST_F(dead_code_eliminate_test, some_dead_channels_all_flags_used)
+{
+   const vec4_builder bld = vec4_builder(v).at_end();
+   src_reg r1 = src_reg(v, glsl_type::vec4_type);
+   src_reg r2 = src_reg(v, glsl_type::vec4_type);
+   src_reg r3 = src_reg(v, glsl_type::vec4_type);
+   src_reg r4 = src_reg(v, glsl_type::vec4_type);
+   src_reg r5 = src_reg(v, glsl_type::vec4_type);
+   src_reg r6 = src_reg(v, glsl_type::vec4_type);
+
+   /* Sequence like the following should not be modified by DCE.
+    *
+    *     cmp.l.f0(8)     g4<1>F         g2<4,4,1>.wF   g1<4,4,1>.xF
+    *     mov(8)          g5<1>.xF       g4<4,4,1>.xF
+    *     (+f0.x) sel(8)  g6<1>UD        g3<4>UD        g6<4>UD
+    */
+   vec4_instruction *test_cmp =
+      bld.CMP(dst_reg(r4), r2, r1, BRW_CONDITIONAL_L);
+
+   test_cmp->src[0].swizzle = BRW_SWIZZLE_WWWW;
+   test_cmp->src[1].swizzle = BRW_SWIZZLE_XXXX;
+
+   vec4_instruction *test_mov =
+      bld.MOV(dst_reg(r5), r4);
+
+   test_mov->dst.writemask = WRITEMASK_X;
+   test_mov->src[0].swizzle = BRW_SWIZZLE_XXXX;
+
+   vec4_instruction *test_sel =
+      bld.SEL(dst_reg(r6), r3, r6);
+
+   set_predicate(BRW_PREDICATE_NORMAL, test_sel);
+
+   /* The scratch write is here just to make r5 and r6 be live so that the
+    * whole program doesn't get eliminated by DCE.
+    */
+   v->emit(v->SCRATCH_WRITE(dst_reg(r4), r6, r5));
+
+   dead_code_eliminate(v);
+
+   EXPECT_EQ(test_cmp->dst.writemask, WRITEMASK_XYZW);
+}