KonstantinSeurer
/
mesa
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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@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@globallogic.com>
Reviewed-by: Rhys Perry <pendingchaos02@gmail.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6186>
This commit is contained in:
Danylo Piliaiev 2020-08-04 18:12:47 +03:00 committed by Marge Bot
parent 95b9fc4146
commit 8077f3f4c4
5 changed files with 238 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
src/compiler/nir/meson.build
View File

@ -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

8
src/compiler/nir/nir_control_flow.c
View File

@ -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);
}
}
}

3
src/compiler/nir/nir_control_flow.h
View File

@ -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

5
src/compiler/nir/nir_lower_returns.c
View File

@ -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);

213
src/compiler/nir/tests/lower_returns_tests.cpp Normal file
View File

@ -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);
}