<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 2, 2018 at 7:30 AM Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Oct 2, 2018 at 5:53 AM Iago Toral <<a href="mailto:itoral@igalia.com" target="_blank">itoral@igalia.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, 2018-09-22 at 16:39 -0500, Jason Ekstrand wrote:<br>
> If the block in which the jump is inserted is the predecessor of a<br>
> phi<br>
> then we need to remove phi sources otherwise the phi may end up with<br>
> things improperly connected.  Found by running the Vulkan CTS with<br>
> SPIR-V optimizations enabled.<br>
> <br>
> Cc: <a href="mailto:mesa-stable@lists.freedesktop.org" target="_blank">mesa-stable@lists.freedesktop.org</a><br>
> ---<br>
>  src/compiler/nir/nir_control_flow.c | 36 +++++++++++++++----------<br>
> ----<br>
>  1 file changed, 19 insertions(+), 17 deletions(-)<br>
> <br>
> diff --git a/src/compiler/nir/nir_control_flow.c<br>
> b/src/compiler/nir/nir_control_flow.c<br>
> index 3b0a0f1a5b0..a82f35550b8 100644<br>
> --- a/src/compiler/nir/nir_control_flow.c<br>
> +++ b/src/compiler/nir/nir_control_flow.c<br>
> @@ -437,6 +437,23 @@ nearest_loop(nir_cf_node *node)<br>
>     return nir_cf_node_as_loop(node);<br>
>  }<br>
>  <br>
> +static void<br>
> +remove_phi_src(nir_block *block, nir_block *pred)<br>
> +{<br>
> +   nir_foreach_instr(instr, block) {<br>
> +      if (instr->type != nir_instr_type_phi)<br>
> +         break;<br>
> +<br>
> +      nir_phi_instr *phi = nir_instr_as_phi(instr);<br>
> +      nir_foreach_phi_src_safe(src, phi) {<br>
> +         if (src->pred == pred) {<br>
> +            list_del(&src->src.use_link);<br>
> +            exec_node_remove(&src->node);<br>
> +         }<br>
> +      }<br>
> +   }<br>
> +}<br>
> +<br>
>  /*<br>
>   * update the CFG after a jump instruction has been added to the end<br>
> of a block<br>
>   */<br>
> @@ -447,6 +464,8 @@ nir_handle_add_jump(nir_block *block)<br>
>     nir_instr *instr = nir_block_last_instr(block);<br>
>     nir_jump_instr *jump_instr = nir_instr_as_jump(instr);<br>
>  <br>
> +   if (block->successors[0])<br>
> +      remove_phi_src(block->successors[0], block);<br>
<br>
Don't we need to do the same for block->successors[1]?<br></blockquote><div><br></div><div>I was going to say no because his function handles *adding* a phi and so the block should already only have one successor.  However, I suppose you could add a phi right before an if.  I'll add the one for block->successors[1] just to be safe.</div></div></div></blockquote><div><br></div><div>On further thought, I don't think it's possible to end up with phi sources at block->successors[1].  The only type of block that can have multiple successors is one right before an if and both sides of the if have only one predecessor so they can't have phis.  Unless, of course, we add a bunch of no-op phis for some reason.  Eh, removing phis on block->successors[1] is harmless and probably more correct.  Still, it's a very weird case...</div><div><br></div><div>--Jason<br></div></div></div>