<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Fri, Sep 14, 2018 at 10:46 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Note at the moment the pass called is nir_opt_copy_prop_vars, because dead write elimination is implemented there. Also added tests that involve identifying dead writes in multiple blocks (e.g. the overwrite happens in another block). Those currently fail as expected, so are marked to be skipped. --- src/compiler/nir/tests/vars_tests.cpp | 241 ++++++++++++++++++++++++++ 1 file changed, 241 insertions(+) diff --git a/src/compiler/nir/tests/vars_tests.cpp b/src/compiler/nir/tests/vars_tests.cpp index 7fbdb514349..dd913f04429 100644 --- a/src/compiler/nir/tests/vars_tests.cpp +++ b/src/compiler/nir/tests/vars_tests.cpp @@ -26,6 +26,9 @@ #include "nir.h" #include "nir_builder.h" +/* This optimization is done together with copy propagation. */ +#define nir_opt_dead_write_vars nir_opt_copy_prop_vars + namespace { class nir_vars_test : public ::testing::Test { @@ -141,6 +144,7 @@ nir_imm_ivec2(nir_builder *build, int x, int y) /* Allow grouping the tests while still sharing the helpers. */ class nir_copy_prop_vars_test : public nir_vars_test {}; +class nir_dead_write_vars_test : public nir_vars_test {}; } // namespace @@ -197,3 +201,240 @@ TEST_F(nir_copy_prop_vars_test, simple_store_load) EXPECT_EQ(store->src[1].ssa, stored_value); } } + +TEST_F(nir_dead_write_vars_test, no_dead_writes_in_block) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 2); </blockquote><div> </div><div>Using inputs and outputs is probably a tad bit safer than shader_storage but I don't think it matters too much. This is probably fine. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_FALSE(progress); +} + +TEST_F(nir_dead_write_vars_test, no_dead_writes_different_components_in_block) +{ + nir_variable **v = create_many_ivec2(nir_var_shader_storage, "v", 3); + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1 << 0); + nir_store_var(b, v[0], nir_load_var(b, v[2]), 1 << 1); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_FALSE(progress); +} + +TEST_F(nir_dead_write_vars_test, no_dead_writes_in_if_statement) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 6); + + nir_store_var(b, v[2], nir_load_var(b, v[0]), 1); + nir_store_var(b, v[3], nir_load_var(b, v[1]), 1); + + /* Each arm of the if statement will overwrite one store. */ + nir_if *if_stmt = nir_push_if(b, nir_imm_int(b, 0)); </blockquote><div> </div><div>Maybe nir_push_if(b, nir_load_var(b, v[0])); so that it's not a loop with one dead side. I doubt this pass will ever get that smart but it's easy enough to just prevent the possibility. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + nir_store_var(b, v[2], nir_load_var(b, v[4]), 1); + + nir_push_else(b, if_stmt); + nir_store_var(b, v[3], nir_load_var(b, v[5]), 1); + + nir_pop_if(b, if_stmt); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_FALSE(progress); +} + +TEST_F(nir_dead_write_vars_test, no_dead_writes_in_loop_statement) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 3); + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1); + + /* Loop will write other value. Since it might not be executed, it doesn't + * kill the first write. + */ + nir_loop *loop = nir_push_loop(b); + + nir_if *if_stmt = nir_push_if(b, nir_imm_int(b, 0)); </blockquote><div> </div><div>Same here, though you'll want to use v[1] instead of v[0] so it doesn't get copy-propagated. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + nir_jump(b, nir_jump_break); + nir_pop_if(b, if_stmt); + + nir_store_var(b, v[0], nir_load_var(b, v[2]), 1); + nir_pop_loop(b, loop); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_FALSE(progress); +} + +TEST_F(nir_dead_write_vars_test, dead_write_in_block) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 3); + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1); + nir_ssa_def *load_v2 = nir_load_var(b, v[2]); + nir_store_var(b, v[0], load_v2, 1); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + + EXPECT_EQ(1, count_intrinsics(nir_intrinsic_store_deref)); + + nir_intrinsic_instr *store = find_next_intrinsic(nir_intrinsic_store_deref, NULL); + ASSERT_TRUE(store->src[1].is_ssa); + EXPECT_EQ(store->src[1].ssa, load_v2); +} + +TEST_F(nir_dead_write_vars_test, dead_write_components_in_block) +{ + nir_variable **v = create_many_ivec2(nir_var_shader_storage, "v", 3); + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1 << 0); + nir_ssa_def *load_v2 = nir_load_var(b, v[2]); + nir_store_var(b, v[0], load_v2, 1 << 0); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + + EXPECT_EQ(1, count_intrinsics(nir_intrinsic_store_deref)); + + nir_intrinsic_instr *store = find_next_intrinsic(nir_intrinsic_store_deref, NULL); + ASSERT_TRUE(store->src[1].is_ssa); + EXPECT_EQ(store->src[1].ssa, load_v2); +} + + +/* TODO: The DISABLED tests below depend on the dead write removal be able to + * identify dead writes between multiple blocks. This is still not + * implemented. + */ + +TEST_F(nir_dead_write_vars_test, DISABLED_dead_write_in_two_blocks) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 3); + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1); + nir_ssa_def *load_v2 = nir_load_var(b, v[2]); + + /* Causes the stores to be in different blocks. */ + nir_pop_if(b, nir_push_if(b, nir_imm_int(b, 0))); + + nir_store_var(b, v[0], load_v2, 1); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + + EXPECT_EQ(1, count_intrinsics(nir_intrinsic_store_deref)); + + nir_intrinsic_instr *store = find_next_intrinsic(nir_intrinsic_store_deref, NULL); + ASSERT_TRUE(store->src[1].is_ssa); + EXPECT_EQ(store->src[1].ssa, load_v2); +} + +TEST_F(nir_dead_write_vars_test, DISABLED_dead_write_components_in_two_blocks) +{ + nir_variable **v = create_many_ivec2(nir_var_shader_storage, "v", 3); + + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1 << 0); + + /* Causes the stores to be in different blocks. */ + nir_pop_if(b, nir_push_if(b, nir_imm_int(b, 0))); + + nir_ssa_def *load_v2 = nir_load_var(b, v[2]); + nir_store_var(b, v[0], load_v2, 1 << 0); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + + EXPECT_EQ(1, count_intrinsics(nir_intrinsic_store_deref)); + + nir_intrinsic_instr *store = find_next_intrinsic(nir_intrinsic_store_deref, NULL); + ASSERT_TRUE(store->src[1].is_ssa); + EXPECT_EQ(store->src[1].ssa, load_v2); +} + +TEST_F(nir_dead_write_vars_test, DISABLED_dead_writes_in_if_statement) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 4); + + /* Both branches will overwrite, making the previous store dead. */ + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1); + + nir_if *if_stmt = nir_push_if(b, nir_imm_int(b, 0)); </blockquote><div> </div><div>Load something non-zero? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + nir_ssa_def *load_v2 = nir_load_var(b, v[2]); + nir_store_var(b, v[0], load_v2, 1); + + nir_push_else(b, if_stmt); + nir_ssa_def *load_v3 = nir_load_var(b, v[3]); + nir_store_var(b, v[0], load_v3, 1); + + nir_pop_if(b, if_stmt); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + EXPECT_EQ(2, count_intrinsics(nir_intrinsic_store_deref)); + + nir_intrinsic_instr *store = NULL; + store = find_next_intrinsic(nir_intrinsic_store_deref, store); + ASSERT_TRUE(store->src[1].is_ssa); + EXPECT_EQ(store->src[1].ssa, load_v2); + + store = find_next_intrinsic(nir_intrinsic_store_deref, store); + ASSERT_TRUE(store->src[1].is_ssa); + EXPECT_EQ(store->src[1].ssa, load_v3); +} + +TEST_F(nir_dead_write_vars_test, DISABLED_memory_barrier_in_two_blocks) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 2); + + nir_store_var(b, v[0], nir_imm_int(b, 1), 1); + nir_store_var(b, v[1], nir_imm_int(b, 2), 1); + + /* Split into many blocks. */ + nir_pop_if(b, nir_push_if(b, nir_imm_int(b, 0))); </blockquote><div> </div><div>Ok, I'm starting to get the idea that protecting ourselves from control flow optimizations is probably a lost cause. You can ignore all those comments.</div><div> </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + /* Because it is before the barrier, this will kill the previous store to that target. */ + nir_store_var(b, v[0], nir_imm_int(b, 3), 1); + + nir_builder_instr_insert(b, &nir_intrinsic_instr_create(b->shader, nir_intrinsic_memory_barrier)->instr); + + nir_store_var(b, v[1], nir_imm_int(b, 4), 1); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + + EXPECT_EQ(3, count_intrinsics(nir_intrinsic_store_deref)); +} + +TEST_F(nir_dead_write_vars_test, DISABLED_unrelated_barrier_in_two_blocks) +{ + nir_variable **v = create_many_int(nir_var_shader_storage, "v", 3); + nir_variable *out = create_int(nir_var_shader_out, "out"); + + nir_store_var(b, out, nir_load_var(b, v[1]), 1); + nir_store_var(b, v[0], nir_load_var(b, v[1]), 1); + + /* Split into many blocks. */ + nir_pop_if(b, nir_push_if(b, nir_imm_int(b, 0))); + + /* Emit vertex will ensure writes to output variables are considered used, + * but should not affect other types of variables. */ + + nir_builder_instr_insert(b, &nir_intrinsic_instr_create(b->shader, nir_intrinsic_emit_vertex)->instr); + + nir_store_var(b, out, nir_load_var(b, v[2]), 1); + nir_store_var(b, v[0], nir_load_var(b, v[2]), 1); + + bool progress = nir_opt_dead_write_vars(b->shader); + ASSERT_TRUE(progress); + + /* Verify the first write to v[0] was removed. */ + EXPECT_EQ(3, count_intrinsics(nir_intrinsic_store_deref)); + + nir_intrinsic_instr *store = NULL; + store = find_next_intrinsic(nir_intrinsic_store_deref, store); + EXPECT_EQ(nir_intrinsic_get_var(store, 0), out); + store = find_next_intrinsic(nir_intrinsic_store_deref, store); + EXPECT_EQ(nir_intrinsic_get_var(store, 0), out); + store = find_next_intrinsic(nir_intrinsic_store_deref, store); + EXPECT_EQ(nir_intrinsic_get_var(store, 0), v[0]); +} -- 2.19.0 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div>