On Fri, Sep 14, 2018 at 10:46 PM Caio Marcelo de Oliveira Filho <
caio.olive...@intel.com> wrote:
> 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);
>
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.
> +
> + 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));
>
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.
> + 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));
>
Same here, though you'll want to use v[1] instead of v[0] so it doesn't get
copy-propagated.
> + 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));
>
Load something non-zero?
> + 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)));
>
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.
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
> +
> + /* 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
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
