From 5d238c0c2cd90dc175e04de6cf6b3116c440c90b Mon Sep 17 00:00:00 2001 From: Jesse Natalie Date: Mon, 8 May 2023 17:45:49 -0700 Subject: [PATCH] nir_lower_returns: Optimize phis before beginning the pass This adds a little extra work, since now dominance is computed and blocks that don't just have then-return or else-return are looked at. However it means that nir_lower_returns can now keep phis up to date by inserting undefs without causing some phis to become non-trivial. This ends up obviating a couple of tests for lower_returns. Reviewed-by: Alyssa Rosenzweig Part-of: --- src/compiler/nir/meson.build | 1 - src/compiler/nir/nir_lower_returns.c | 13 +- src/compiler/nir/tests/lower_returns_tests.cpp | 209 ------------------------- 3 files changed, 8 insertions(+), 215 deletions(-) delete mode 100644 src/compiler/nir/tests/lower_returns_tests.cpp diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build index adf07d0..c965bdc 100644 --- a/src/compiler/nir/meson.build +++ b/src/compiler/nir/meson.build @@ -416,7 +416,6 @@ if with_tests 'tests/loop_analyze_tests.cpp', 'tests/loop_unroll_tests.cpp', 'tests/lower_alu_width_tests.cpp', - 'tests/lower_returns_tests.cpp', 'tests/mod_analysis_tests.cpp', 'tests/negative_equal_tests.cpp', 'tests/opt_if_tests.cpp', diff --git a/src/compiler/nir/nir_lower_returns.c b/src/compiler/nir/nir_lower_returns.c index d7126cb..f4319c1 100644 --- a/src/compiler/nir/nir_lower_returns.c +++ b/src/compiler/nir/nir_lower_returns.c @@ -132,11 +132,11 @@ lower_returns_in_if(nir_if *if_stmt, struct lower_returns_state *state) */ /* nir_cf_extract will not extract phis at the start of the block. In - * this case we know that any phis will have to have a single - * predecessor, so we can just replace the phi with its single source. + * this case we know that any phis will have to have had a single + * predecessor, and should've been removed by the opt_remove_phis before + * beginning this pass. */ nir_block *succ_block = nir_after_cf_node(&if_stmt->cf_node).block; - nir_opt_remove_phis_block(succ_block); assert(nir_block_first_instr(succ_block) == NULL || nir_block_first_instr(succ_block)->type != nir_instr_type_phi); @@ -298,11 +298,14 @@ nir_lower_returns_impl(nir_function_impl *impl) bool nir_lower_returns(nir_shader *shader) { - bool progress = false; + /* Before removing jumps and adding undef sources to otherwise single-source phis, + * go ahead and simplify those single-source phis. + */ + bool progress = nir_opt_remove_phis(shader); nir_foreach_function(function, shader) { if (function->impl) - progress = nir_lower_returns_impl(function->impl) || progress; + progress |= nir_lower_returns_impl(function->impl) || progress; } return progress; diff --git a/src/compiler/nir/tests/lower_returns_tests.cpp b/src/compiler/nir/tests/lower_returns_tests.cpp deleted file mode 100644 index 27ee3c7..0000000 --- a/src/compiler/nir/tests/lower_returns_tests.cpp +++ /dev/null @@ -1,209 +0,0 @@ -/* - * 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 -#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 = { }; - bld = nir_builder_init_simple_shader(MESA_SHADER_VERTEX, &options, "lower returns test"); - - 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_instr_add_src(phi, pred, nir_src_for_ssa(def)); - - nir_ssa_dest_init(&phi->instr, &phi->dest, def->num_components, - def->bit_size); - - 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); -} -- 2.7.4