<div dir="ltr">Ok... Different set of comments. Now, with less bogosity! <div class="gmail_extra"> <div class="gmail_quote">On Mon, Dec 19, 2016 at 8:18 PM, Timothy Arceri <<a href="mailto:timothy.arceri@collabora.com" target="_blank">timothy.arceri@collabora.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Unless an if statement contains nested returns we can simply add any following instructions to the branch without the return. V2: fix handling if_nested_return value when there is a sibling if/loop that doesn't contain a return. (Spotted by Ken) --- src/compiler/nir/nir_lower_returns.c | 37 ++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/src/compiler/nir/nir_lower_returns.c b/src/compiler/nir/nir_lower_returns.c index cf49d5b..5eec984 100644 --- a/src/compiler/nir/nir_lower_returns.c +++ b/src/compiler/nir/nir_lower_returns.c @@ -30,6 +30,8 @@ struct lower_returns_state { struct exec_list *cf_list; nir_loop *loop; nir_variable *return_flag; + /* Are there other return statments nested in the current if */ + bool if_nested_return; </blockquote><div> </div><div>Can we rename this to has_predicated_return or has_return_in_if? I would prefer has_predicated_return. Also, we should add a comment: </div><div>This indicates that we have a return which is predicated on some form of control-flow. Since whether or not the return happens can only be determined dynamically at run-time, everything that occurs afterwards needs to be predicated on the return flag variable. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> }; static bool lower_returns_in_cf_list(struct exec_list *cf_list, @@ -82,8 +84,10 @@ lower_returns_in_loop(nir_loop *loop, struct lower_returns_state *state) * flag set to true. We need to predicate everything following the loop * on the return flag. */ - if (progress) + if (progress) { predicate_following(&loop->cf_node, state); + state->if_nested_return = true; + } return progress; } @@ -91,10 +95,13 @@ lower_returns_in_loop(nir_loop *loop, struct lower_returns_state *state) static bool lower_returns_in_if(nir_if *if_stmt, struct lower_returns_state *state) { - bool progress; + bool progress, then_progress; - progress = lower_returns_in_cf_list(&if_stmt->then_list, state); - progress = lower_returns_in_cf_list(&if_stmt->else_list, state) || progress; + bool if_nested_return = state->if_nested_return; + state->if_nested_return = false; + + then_progress = lower_returns_in_cf_list(&if_stmt->then_list, state); + progress = lower_returns_in_cf_list(&if_stmt->else_list, state) || then_progress; </blockquote><div> </div><div>I don't think this is what you want. We really want two separate then_progress and else_progress booleans. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> /* If either of the recursive calls made progress, then there were * returns inside of the body of the if. If we're in a loop, then these @@ -106,8 +113,25 @@ lower_returns_in_if(nir_if *if_stmt, struct lower_returns_state *state) * after a return, we need to predicate everything following on the * return flag. */ - if (progress && !state->loop) - predicate_following(&if_stmt->cf_node, state); + if (progress && !state->loop) { + if (state->if_nested_return) { + predicate_following(&if_stmt->cf_node, state); + } else { + /* If there are no nested returns we can just add the instructions to + * the end of the branch that doesn't have the return. + */ + nir_cf_list list; + nir_cf_extract(&list, nir_after_cf_node(&if_stmt->cf_node), + nir_after_cf_list(state->cf_list)); + + if (then_progress) + nir_cf_reinsert(&list, nir_after_cf_list(&if_stmt->else_list)); + else + nir_cf_reinsert(&list, nir_after_cf_list(&if_stmt->then_list)); </blockquote><div> </div><div>What if both make progress? In that case, we should probbly just delete the "after" code. This can happen with shaders coming in from SPIR-V since this pass is run *before* dead_cf. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + } + + state->if_nested_return = progress || if_nested_return; return progress; } @@ -221,6 +245,7 @@ nir_lower_returns_impl(nir_function_impl *impl) state.cf_list = &impl->body; state.loop = NULL; state.return_flag = NULL; + state.if_nested_return = false; nir_builder_init(&state.builder, impl); bool progress = lower_returns_in_cf_list(&impl->body, &state); -- 2.9.3 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>