Mesa (master): nir/lower_returns: Append missing phis' sources after "break" insertion
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Mon Nov 2 14:28:51 UTC 2020
Module: Mesa
Branch: master
Commit: 8077f3f4c4a3d8007caa30eed93fed1c6bbf3c5a
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=8077f3f4c4a3d8007caa30eed93fed1c6bbf3c5a
Author: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
Date: Tue Aug 4 18:12:47 2020 +0300
nir/lower_returns: Append missing phis' sources after "break" insertion
After we lowered `return` into `break` - the control flow is changed and
the block with this change has a new successor, which means that in this
new successor phis should have additional source.
Since the instructions that use phis in the successor are predicated -
it's ok for a new phi source to be undef.
If `return` is lowered in a nested loop, `break` is inserted in the outer
loops, so all new blocks with break require the same changes to phis
described above.
Examples of NIR before lowering:
block block_0:
loop {
block block_1:
if ssa_2 {
block block_2:
return
// succs: block_6
} else {
block block_2:
break;
// succs: block_5
}
block block_4:
}
block block_5:
// preds: block_3
vec1 32 ssa_4 = phi block_3: ssa_1
// succs: block_6
block block_6:
Here converting return to break should add block_2 to the phis
of block_5.
block block_0:
loop {
block block_1:
loop {
block block_2:
if ssa_2 {
block block_3:
return
// succs: block_8
} else {
block block_4:
break;
// succs: block_6
}
block block_5:
}
block block_6:
break;
// succs: block_7
}
block block_7:
// preds: block_6
vec1 32 ssa_4 = phi block_6: ssa_1
// succs: block_8
block block_8:
Here converting return to break will insert conditional break in
the outer loop, changing block_6 predcessors.
Cc: <mesa-stable at lists.freedesktop.org>
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/3322
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/3498
Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
Reviewed-by: Rhys Perry <pendingchaos02 at gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6186>
---
src/compiler/nir/meson.build | 13 ++
src/compiler/nir/nir_control_flow.c | 8 +-
src/compiler/nir/nir_control_flow.h | 3 +
src/compiler/nir/nir_lower_returns.c | 5 +
src/compiler/nir/tests/lower_returns_tests.cpp | 213 +++++++++++++++++++++++++
5 files changed, 238 insertions(+), 4 deletions(-)
diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
index d9899d29fc2..dad896dcf75 100644
--- a/src/compiler/nir/meson.build
+++ b/src/compiler/nir/meson.build
@@ -458,4 +458,17 @@ if with_tests
),
suite : ['compiler', 'nir'],
)
+
+ test(
+ 'nir_lower_returns',
+ executable(
+ 'nir_lower_returns_tests',
+ files('tests/lower_returns_tests.cpp'),
+ cpp_args : [cpp_msvc_compat_args],
+ gnu_symbol_visibility : 'hidden',
+ include_directories : [inc_include, inc_src, inc_mapi, inc_mesa, inc_gallium, inc_gallium_aux],
+ dependencies : [dep_thread, idep_gtest, idep_nir, idep_mesautil],
+ ),
+ suite : ['compiler', 'nir'],
+ )
endif
diff --git a/src/compiler/nir/nir_control_flow.c b/src/compiler/nir/nir_control_flow.c
index 75146cc1cf3..9d5f52000d7 100644
--- a/src/compiler/nir/nir_control_flow.c
+++ b/src/compiler/nir/nir_control_flow.c
@@ -226,8 +226,8 @@ rewrite_phi_preds(nir_block *block, nir_block *old_pred, nir_block *new_pred)
}
}
-static void
-insert_phi_undef(nir_block *block, nir_block *pred)
+void
+nir_insert_phi_undef(nir_block *block, nir_block *pred)
{
nir_function_impl *impl = nir_cf_node_get_function(&block->cf_node);
nir_foreach_instr(instr, block) {
@@ -298,7 +298,7 @@ block_add_normal_succs(nir_block *block)
nir_block *head_block = nir_loop_first_block(loop);
link_blocks(block, head_block, NULL);
- insert_phi_undef(head_block, block);
+ nir_insert_phi_undef(head_block, block);
} else {
nir_function_impl *impl = nir_cf_node_as_function(parent);
link_blocks(block, impl->end_block, NULL);
@@ -318,7 +318,7 @@ block_add_normal_succs(nir_block *block)
nir_block *first_block = nir_loop_first_block(next_loop);
link_blocks(block, first_block, NULL);
- insert_phi_undef(first_block, block);
+ nir_insert_phi_undef(first_block, block);
}
}
}
diff --git a/src/compiler/nir/nir_control_flow.h b/src/compiler/nir/nir_control_flow.h
index 9111b30a297..c1867a81180 100644
--- a/src/compiler/nir/nir_control_flow.h
+++ b/src/compiler/nir/nir_control_flow.h
@@ -171,6 +171,9 @@ nir_cf_node_remove(nir_cf_node *node)
nir_cf_delete(&list);
}
+/** inserts undef phi sources from predcessor into phis of the block */
+void nir_insert_phi_undef(nir_block *block, nir_block *pred);
+
#ifdef __cplusplus
}
#endif
diff --git a/src/compiler/nir/nir_lower_returns.c b/src/compiler/nir/nir_lower_returns.c
index ae933e662cd..76b320b8042 100644
--- a/src/compiler/nir/nir_lower_returns.c
+++ b/src/compiler/nir/nir_lower_returns.c
@@ -62,6 +62,9 @@ predicate_following(nir_cf_node *node, struct lower_returns_state *state)
* conditional break.
*/
nir_jump(b, nir_jump_break);
+
+ nir_block *block = nir_cursor_current_block(b->cursor);
+ nir_insert_phi_undef(block->successors[0], block);
} else {
/* Otherwise, we need to actually move everything into the else case
* of the if statement.
@@ -205,6 +208,8 @@ lower_returns_in_block(nir_block *block, struct lower_returns_state *state)
if (state->loop) {
/* We're in a loop; we need to break out of it. */
nir_jump(b, nir_jump_break);
+
+ nir_insert_phi_undef(block->successors[0], block);
} else {
/* Not in a loop; we'll deal with predicating later*/
assert(nir_cf_node_next(&block->cf_node) == NULL);
diff --git a/src/compiler/nir/tests/lower_returns_tests.cpp b/src/compiler/nir/tests/lower_returns_tests.cpp
new file mode 100644
index 00000000000..022ebc32239
--- /dev/null
+++ b/src/compiler/nir/tests/lower_returns_tests.cpp
@@ -0,0 +1,213 @@
+/*
+ * Copyright © 2020 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 "nir.h"
+#include "nir_builder.h"
+
+class nir_opt_lower_returns_test : public ::testing::Test {
+protected:
+ nir_opt_lower_returns_test();
+ ~nir_opt_lower_returns_test();
+
+ nir_builder bld;
+
+ nir_ssa_def *in_def;
+};
+
+nir_opt_lower_returns_test::nir_opt_lower_returns_test()
+{
+ glsl_type_singleton_init_or_ref();
+
+ static const nir_shader_compiler_options options = { };
+ nir_builder_init_simple_shader(&bld, NULL, MESA_SHADER_VERTEX, &options);
+
+ nir_variable *var = nir_variable_create(bld.shader, nir_var_shader_in, glsl_int_type(), "in");
+ in_def = nir_load_var(&bld, var);
+}
+
+nir_opt_lower_returns_test::~nir_opt_lower_returns_test()
+{
+ ralloc_free(bld.shader);
+ glsl_type_singleton_decref();
+}
+
+nir_phi_instr *create_one_source_phi(nir_shader *shader, nir_block *pred,
+ nir_ssa_def *def)
+{
+ nir_phi_instr *phi = nir_phi_instr_create(shader);
+
+ nir_phi_src *phi_src;
+ phi_src = ralloc(phi, nir_phi_src);
+ phi_src->pred = pred;
+ phi_src->src = nir_src_for_ssa(def);
+ exec_list_push_tail(&phi->srcs, &phi_src->node);
+
+ nir_ssa_dest_init(&phi->instr, &phi->dest,
+ def->num_components, def->bit_size, NULL);
+
+ return phi;
+}
+
+TEST_F(nir_opt_lower_returns_test, phis_after_loop)
+{
+ /* Test that after lowering of "return" the phis in block_5
+ * have two sources, because block_2 will have block_5
+ * as a successor.
+ *
+ * block block_0:
+ * loop {
+ * block block_1:
+ * if ssa_2 {
+ * block block_2:
+ * return
+ * // succs: block_6
+ * } else {
+ * block block_3:
+ * break;
+ * // succs: block_5
+ * }
+ * block block_4:
+ * }
+ * block block_5:
+ * // preds: block_3
+ * vec1 32 ssa_4 = phi block_3: ssa_1
+ * vec1 32 ssa_5 = phi block_3: ssa_1
+ * // succs: block_6
+ * block block_6:
+ */
+
+ nir_loop *loop = nir_push_loop(&bld);
+
+ nir_ssa_def *one = nir_imm_int(&bld, 1);
+
+ nir_ssa_def *cmp_result = nir_ieq(&bld, in_def, one);
+ nir_if *nif = nir_push_if(&bld, cmp_result);
+
+ nir_jump(&bld, nir_jump_return);
+
+ nir_push_else(&bld, NULL);
+
+ nir_jump(&bld, nir_jump_break);
+
+ nir_pop_if(&bld, NULL);
+
+ nir_block *else_block = nir_if_last_else_block(nif);
+
+ nir_pop_loop(&bld, loop);
+
+ bld.cursor = nir_after_cf_node_and_phis(&loop->cf_node);
+
+ nir_phi_instr *const phi_1 =
+ create_one_source_phi(bld.shader, else_block, one);
+ nir_builder_instr_insert(&bld, &phi_1->instr);
+
+ nir_phi_instr *const phi_2 =
+ create_one_source_phi(bld.shader, else_block, one);
+ nir_builder_instr_insert(&bld, &phi_2->instr);
+
+ ASSERT_TRUE(nir_lower_returns(bld.shader));
+ EXPECT_EQ(phi_1->srcs.length(), 2);
+ EXPECT_EQ(phi_2->srcs.length(), 2);
+
+ nir_validate_shader(bld.shader, NULL);
+}
+
+TEST_F(nir_opt_lower_returns_test, phis_after_outer_loop)
+{
+ /* Test that after lowering of "return" the phis in block_7
+ * have two sources, because block_6 will have a conditional break
+ * inserted, which will add a new predcessor to block_7.
+ *
+ * block block_0:
+ * loop {
+ * block block_1:
+ * loop {
+ * block block_2:
+ * if ssa_2 {
+ * block block_3:
+ * return
+ * // succs: block_8
+ * } else {
+ * block block_4:
+ * break;
+ * // succs: block_6
+ * }
+ * block block_5:
+ * }
+ * block block_6:
+ * break;
+ * // succs: block_7
+ * }
+ * block block_7:
+ * // preds: block_6
+ * vec1 32 ssa_4 = phi block_6: ssa_1
+ * vec1 32 ssa_5 = phi block_6: ssa_1
+ * // succs: block_8
+ * block block_8:
+ */
+
+ nir_loop *loop_outer = nir_push_loop(&bld);
+
+ bld.cursor = nir_after_cf_list(&loop_outer->body);
+
+ nir_loop *loop_inner = nir_push_loop(&bld);
+
+ bld.cursor = nir_after_cf_list(&loop_inner->body);
+
+ nir_ssa_def *one = nir_imm_int(&bld, 1);
+
+ nir_ssa_def *cmp_result = nir_ieq(&bld, in_def, one);
+ nir_push_if(&bld, cmp_result);
+
+ nir_jump(&bld, nir_jump_return);
+
+ nir_push_else(&bld, NULL);
+
+ nir_jump(&bld, nir_jump_break);
+
+ nir_pop_if(&bld, NULL);
+
+ nir_pop_loop(&bld, loop_inner);
+
+ bld.cursor = nir_after_cf_node_and_phis(&loop_inner->cf_node);
+
+ nir_jump(&bld, nir_jump_break);
+
+ nir_pop_loop(&bld, loop_outer);
+
+ bld.cursor = nir_after_cf_node_and_phis(&loop_outer->cf_node);
+
+ nir_phi_instr *const phi_1 =
+ create_one_source_phi(bld.shader, nir_loop_last_block(loop_outer), one);
+ nir_builder_instr_insert(&bld, &phi_1->instr);
+
+ nir_phi_instr *const phi_2 =
+ create_one_source_phi(bld.shader, nir_loop_last_block(loop_outer), one);
+ nir_builder_instr_insert(&bld, &phi_2->instr);
+
+ ASSERT_TRUE(nir_lower_returns(bld.shader));
+ EXPECT_EQ(phi_1->srcs.length(), 2);
+ EXPECT_EQ(phi_2->srcs.length(), 2);
+
+ nir_validate_shader(bld.shader, NULL);
+}
More information about the mesa-commit
mailing list