Mesa (staging/20.0): nir/copy_prop_vars: Report progress when deleting self-copies
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Wed Apr 29 23:14:23 UTC 2020
Module: Mesa
Branch: staging/20.0
Commit: 60c2ae0240ae512966fad22a3a13d6488f0bbc6c
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=60c2ae0240ae512966fad22a3a13d6488f0bbc6c
Author: Jason Ekstrand <jason at jlekstrand.net>
Date: Mon Apr 27 10:38:31 2020 -0500
nir/copy_prop_vars: Report progress when deleting self-copies
Fixes: 62332d139c8f6 "nir: Add a local variable-based copy prop..."
Reviewed-by: Caio Marcelo de Oliveira Filho <caio.oliveira at intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4767>
(cherry picked from commit ed677171675fe8ee204deac1e2089f480681b1b4)
---
.pick_status.json | 2 +-
src/compiler/nir/nir_opt_copy_prop_vars.c | 1 +
src/compiler/nir/tests/vars_tests.cpp | 137 ++++++++++++++++++++++++++++++
3 files changed, 139 insertions(+), 1 deletion(-)
diff --git a/.pick_status.json b/.pick_status.json
index 0516349a6c7..9eeb623717c 100644
--- a/.pick_status.json
+++ b/.pick_status.json
@@ -1219,7 +1219,7 @@
"description": "nir/copy_prop_vars: Report progress when deleting self-copies",
"nominated": true,
"nomination_type": 1,
- "resolution": 0,
+ "resolution": 1,
"master_sha": null,
"because_sha": "62332d139c8f6deb7fd8b72a48b34b4b652df7c1"
},
diff --git a/src/compiler/nir/nir_opt_copy_prop_vars.c b/src/compiler/nir/nir_opt_copy_prop_vars.c
index 05bdec50f45..158b788e1f1 100644
--- a/src/compiler/nir/nir_opt_copy_prop_vars.c
+++ b/src/compiler/nir/nir_opt_copy_prop_vars.c
@@ -987,6 +987,7 @@ copy_prop_vars_block(struct copy_prop_var_state *state,
if (nir_compare_derefs(src, dst) & nir_derefs_equal_bit) {
/* This is a no-op self-copy. Get rid of it */
nir_instr_remove(instr);
+ state->progress = true;
continue;
}
diff --git a/src/compiler/nir/tests/vars_tests.cpp b/src/compiler/nir/tests/vars_tests.cpp
index e660b86ebe6..8d35587b18c 100644
--- a/src/compiler/nir/tests/vars_tests.cpp
+++ b/src/compiler/nir/tests/vars_tests.cpp
@@ -211,6 +211,21 @@ scoped_memory_barrier(nir_builder *b,
} // namespace
+static nir_ssa_def *
+nir_load_var_volatile(nir_builder *b, nir_variable *var)
+{
+ return nir_load_deref_with_access(b, nir_build_deref_var(b, var),
+ ACCESS_VOLATILE);
+}
+
+static void
+nir_store_var_volatile(nir_builder *b, nir_variable *var,
+ nir_ssa_def *value, nir_component_mask_t writemask)
+{
+ nir_store_deref_with_access(b, nir_build_deref_var(b, var),
+ value, writemask, ACCESS_VOLATILE);
+}
+
TEST_F(nir_redundant_load_vars_test, duplicated_load)
{
/* Load a variable twice in the same block. One should be removed. */
@@ -233,6 +248,41 @@ TEST_F(nir_redundant_load_vars_test, duplicated_load)
ASSERT_EQ(count_intrinsics(nir_intrinsic_load_deref), 1);
}
+TEST_F(nir_redundant_load_vars_test, duplicated_load_volatile)
+{
+ /* Load a variable twice in the same block. One should be removed. */
+
+ nir_variable *in = create_int(nir_var_shader_in, "in");
+ nir_variable **out = create_many_int(nir_var_shader_out, "out", 3);
+
+ /* Volatile prevents us from eliminating a load by combining it with
+ * another. It shouldn't however, prevent us from combing other
+ * non-volatile loads.
+ */
+ nir_store_var(b, out[0], nir_load_var(b, in), 1);
+ nir_store_var(b, out[1], nir_load_var_volatile(b, in), 1);
+ nir_store_var(b, out[2], nir_load_var(b, in), 1);
+
+ nir_validate_shader(b->shader, NULL);
+
+ ASSERT_EQ(count_intrinsics(nir_intrinsic_load_deref), 3);
+
+ bool progress = nir_opt_copy_prop_vars(b->shader);
+ EXPECT_TRUE(progress);
+
+ nir_validate_shader(b->shader, NULL);
+
+ ASSERT_EQ(count_intrinsics(nir_intrinsic_load_deref), 2);
+
+ nir_intrinsic_instr *first_store = get_intrinsic(nir_intrinsic_store_deref, 0);
+ ASSERT_TRUE(first_store->src[1].is_ssa);
+
+ nir_intrinsic_instr *third_store = get_intrinsic(nir_intrinsic_store_deref, 2);
+ ASSERT_TRUE(third_store->src[1].is_ssa);
+
+ EXPECT_EQ(first_store->src[1].ssa, third_store->src[1].ssa);
+}
+
TEST_F(nir_redundant_load_vars_test, duplicated_load_in_two_blocks)
{
/* Load a variable twice in different blocks. One should be removed. */
@@ -356,6 +406,22 @@ TEST_F(nir_copy_prop_vars_test, simple_copies)
EXPECT_EQ(first_copy->src[1].ssa, second_copy->src[1].ssa);
}
+TEST_F(nir_copy_prop_vars_test, self_copy)
+{
+ nir_variable *v = create_int(nir_var_mem_ssbo, "v");
+
+ nir_copy_var(b, v, v);
+
+ nir_validate_shader(b->shader, NULL);
+
+ bool progress = nir_opt_copy_prop_vars(b->shader);
+ EXPECT_TRUE(progress);
+
+ nir_validate_shader(b->shader, NULL);
+
+ ASSERT_EQ(count_intrinsics(nir_intrinsic_copy_deref), 0);
+}
+
TEST_F(nir_copy_prop_vars_test, simple_store_load)
{
nir_variable **v = create_many_ivec2(nir_var_function_temp, "v", 2);
@@ -485,6 +551,77 @@ TEST_F(nir_copy_prop_vars_test, store_store_load_different_components_in_many_bl
ASSERT_EQ(nir_src_comp_as_uint(store_to_v1->src[1], 1), 20);
}
+TEST_F(nir_copy_prop_vars_test, store_volatile)
+{
+ nir_variable **v = create_many_ivec2(nir_var_function_temp, "v", 2);
+ unsigned mask = 1 | 2;
+
+ nir_ssa_def *first_value = nir_imm_ivec2(b, 10, 20);
+ nir_store_var(b, v[0], first_value, mask);
+
+ nir_ssa_def *second_value = nir_imm_ivec2(b, 30, 40);
+ nir_store_var_volatile(b, v[0], second_value, mask);
+
+ nir_ssa_def *third_value = nir_imm_ivec2(b, 50, 60);
+ nir_store_var(b, v[0], third_value, mask);
+
+ nir_ssa_def *read_value = nir_load_var(b, v[0]);
+ nir_store_var(b, v[1], read_value, mask);
+
+ nir_validate_shader(b->shader, NULL);
+
+ bool progress = nir_opt_copy_prop_vars(b->shader);
+ EXPECT_TRUE(progress);
+
+ nir_validate_shader(b->shader, NULL);
+
+ ASSERT_EQ(count_intrinsics(nir_intrinsic_store_deref), 4);
+
+ /* Our approach here is a bit scorched-earth. We expect the volatile store
+ * in the middle to cause both that store and the one before it to be kept.
+ * Technically, volatile only prevents combining the volatile store with
+ * another store and one could argue that the store before the volatile and
+ * the one after it could be combined. However, it seems safer to just
+ * treat a volatile store like an atomic and prevent any combining across
+ * it.
+ */
+ nir_intrinsic_instr *store_to_v1 = get_intrinsic(nir_intrinsic_store_deref, 3);
+ ASSERT_EQ(nir_intrinsic_get_var(store_to_v1, 0), v[1]);
+ ASSERT_TRUE(store_to_v1->src[1].is_ssa);
+ EXPECT_EQ(store_to_v1->src[1].ssa, third_value);
+}
+
+TEST_F(nir_copy_prop_vars_test, self_copy_volatile)
+{
+ nir_variable *v = create_int(nir_var_mem_ssbo, "v");
+
+ nir_copy_var(b, v, v);
+ nir_copy_deref_with_access(b, nir_build_deref_var(b, v),
+ nir_build_deref_var(b, v),
+ (gl_access_qualifier)0, ACCESS_VOLATILE);
+ nir_copy_deref_with_access(b, nir_build_deref_var(b, v),
+ nir_build_deref_var(b, v),
+ ACCESS_VOLATILE, (gl_access_qualifier)0);
+ nir_copy_var(b, v, v);
+
+ nir_validate_shader(b->shader, NULL);
+
+ bool progress = nir_opt_copy_prop_vars(b->shader);
+ EXPECT_TRUE(progress);
+
+ nir_validate_shader(b->shader, NULL);
+
+ ASSERT_EQ(count_intrinsics(nir_intrinsic_copy_deref), 2);
+
+ /* Store to v[1] should use second_value directly. */
+ nir_intrinsic_instr *first = get_intrinsic(nir_intrinsic_copy_deref, 0);
+ nir_intrinsic_instr *second = get_intrinsic(nir_intrinsic_copy_deref, 1);
+ ASSERT_EQ(nir_intrinsic_src_access(first), ACCESS_VOLATILE);
+ ASSERT_EQ(nir_intrinsic_dst_access(first), (gl_access_qualifier)0);
+ ASSERT_EQ(nir_intrinsic_src_access(second), (gl_access_qualifier)0);
+ ASSERT_EQ(nir_intrinsic_dst_access(second), ACCESS_VOLATILE);
+}
+
TEST_F(nir_copy_prop_vars_test, memory_barrier_in_two_blocks)
{
nir_variable **v = create_many_int(nir_var_mem_ssbo, "v", 4);
More information about the mesa-commit
mailing list