From 440c051340669e809511c05370d6d703c70f6d0e Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Wed, 20 Jun 2018 17:18:30 -0700 Subject: [PATCH] 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 Reviewed-by: Lionel Landwerlin Fixes: 5df88c20 ("i965/vec4: Rewrite dead code elimination to use live in/out.") --- src/intel/Makefile.compiler.am | 5 + .../compiler/brw_vec4_dead_code_eliminate.cpp | 47 ++++- src/intel/compiler/meson.build | 3 +- .../test_vec4_dead_code_eliminate.cpp | 163 ++++++++++++++++++ 4 files changed, 208 insertions(+), 10 deletions(-) create mode 100644 src/intel/compiler/test_vec4_dead_code_eliminate.cpp diff --git a/src/intel/Makefile.compiler.am b/src/intel/Makefile.compiler.am index cd7e6882fb9..7c33e35816b 100644 --- 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 c09a3d7ebe9..99e4c9cacaf 100644 --- 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 2124278cc04..69ce2eab4cf 100644 --- 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 index 00000000000..25739c2895a --- /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 +#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); +} -- 2.30.2