<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 24, 2016 at 9:04 PM, Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Prior to this commit rename_variables_block() is recursively called,<br>
performing a depth-first traversal of the control flow graph. The<br>
function uses a non-trivial amount of stack space for local variables,<br>
which puts us in danger of smashing the stack, given a sufficiently deep<br>
dominance tree.<br>
<br>
XCOM: Enemy Within contains a shader with such a dominance tree (1574<br>
nir_blocks in total, depth of at least 143).<br>
<br>
Jason tells me that he believes that any walk over the nir_blocks that<br>
respects dominance is sufficient (a DFS might have been necessary prior<br>
to the introduction of nir_phi_builder).<br>
<br>
In fact, the introduction of nir_phi_builder made the problem worse:<br>
rename_variables_block(), walks to the bottom of the dominance tree<br>
before calling nir_phi_builder_value_get_<wbr>block_def() which walks back to<br>
the top of the dominance tree...<br>
<br>
In any case, this patch ensures we avoid that problem as well.<br>
<br>
Cc: <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.<wbr>org</a><br>
Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=97225" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/<wbr>show_bug.cgi?id=97225</a><br>
---<br>
Most easily reviewed with git show -w<br></blockquote><div><br></div><div>Yes, yes it is. :)<br><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
 src/compiler/nir/nir_lower_<wbr>vars_to_ssa.c | 207 +++++++++++++++---------------<wbr>-<br>
 1 file changed, 103 insertions(+), 104 deletions(-)<br>
<br>
diff --git a/src/compiler/nir/nir_lower_<wbr>vars_to_ssa.c b/src/compiler/nir/nir_lower_<wbr>vars_to_ssa.c<br>
index 317647b..e2e13e2 100644<br>
--- a/src/compiler/nir/nir_lower_<wbr>vars_to_ssa.c<br>
+++ b/src/compiler/nir/nir_lower_<wbr>vars_to_ssa.c<br>
@@ -479,133 +479,132 @@ lower_copies_to_load_store(<wbr>struct deref_node *node,<br>
  * SSA def on the stack per block.<br>
  */<br>
 static bool<br>
-rename_variables_block(nir_<wbr>block *block, struct lower_variables_state *state)<br>
+rename_variables(struct lower_variables_state *state)<br>
 {<br>
  Â  nir_builder b;<br>
  Â  nir_builder_init(&b, state->impl);<br>
<br>
-  Â nir_foreach_instr_safe(instr, block) {<br>
-  Â  Â  if (instr->type != nir_instr_type_intrinsic)<br>
-  Â  Â  Â  Â continue;<br>
-<br>
-  Â  Â  nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);<br>
-<br>
-  Â  Â  switch (intrin->intrinsic) {<br>
-  Â  Â  case nir_intrinsic_load_var: {<br>
-  Â  Â  Â  Â struct deref_node *node =<br>
-  Â  Â  Â  Â  Â  get_deref_node(intrin-><wbr>variables[0], state);<br>
-<br>
-  Â  Â  Â  Â if (node == NULL) {<br>
-  Â  Â  Â  Â  Â  /* If we hit this path then we are referencing an invalid<br>
-  Â  Â  Â  Â  Â  Â * value.  Most likely, we unrolled something and are<br>
-  Â  Â  Â  Â  Â  Â * reading past the end of some array.  In any case, this<br>
-  Â  Â  Â  Â  Â  Â * should result in an undefined value.<br>
-  Â  Â  Â  Â  Â  Â */<br>
-  Â  Â  Â  Â  Â  nir_ssa_undef_instr *undef =<br>
-  Â  Â  Â  Â  Â  Â  Â nir_ssa_undef_instr_create(<wbr>state->shader,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  intrin->num_components,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  intrin->dest.ssa.bit_size);<br>
-<br>
-  Â  Â  Â  Â  Â  nir_instr_insert_before(&<wbr>intrin->instr, &undef->instr);<br>
-  Â  Â  Â  Â  Â  nir_instr_remove(&intrin-><wbr>instr);<br>
-<br>
-  Â  Â  Â  Â  Â  nir_ssa_def_rewrite_uses(&<wbr>intrin->dest.ssa,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â nir_src_for_ssa(&undef->def));<br>
+  Â nir_foreach_block(block, state->impl) {<br>
+  Â  Â  nir_foreach_instr_safe(instr, block) {<br>
+  Â  Â  Â  Â if (instr->type != nir_instr_type_intrinsic)<br>
  Â  Â  Â  Â  Â  Â continue;<br>
-  Â  Â  Â  Â }<br>
<br>
-  Â  Â  Â  Â if (!node->lower_to_ssa)<br>
-  Â  Â  Â  Â  Â  continue;<br>
+  Â  Â  Â  Â nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);<br>
+<br>
+  Â  Â  Â  Â switch (intrin->intrinsic) {<br>
+  Â  Â  Â  Â case nir_intrinsic_load_var: {<br>
+  Â  Â  Â  Â  Â  struct deref_node *node =<br>
+  Â  Â  Â  Â  Â  Â  Â get_deref_node(intrin-><wbr>variables[0], state);<br>
+<br>
+  Â  Â  Â  Â  Â  if (node == NULL) {<br>
+  Â  Â  Â  Â  Â  Â  Â /* If we hit this path then we are referencing an invalid<br>
+  Â  Â  Â  Â  Â  Â  Â  * value.  Most likely, we unrolled something and are<br>
+  Â  Â  Â  Â  Â  Â  Â  * reading past the end of some array.  In any case, this<br>
+  Â  Â  Â  Â  Â  Â  Â  * should result in an undefined value.<br>
+  Â  Â  Â  Â  Â  Â  Â  */<br>
+  Â  Â  Â  Â  Â  Â  Â nir_ssa_undef_instr *undef =<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  nir_ssa_undef_instr_create(<wbr>state->shader,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â intrin->num_components,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â intrin->dest.ssa.bit_size);<br>
+<br>
+  Â  Â  Â  Â  Â  Â  Â nir_instr_insert_before(&<wbr>intrin->instr, &undef->instr);<br>
+  Â  Â  Â  Â  Â  Â  Â nir_instr_remove(&intrin-><wbr>instr);<br>
+<br>
+  Â  Â  Â  Â  Â  Â  Â nir_ssa_def_rewrite_uses(&<wbr>intrin->dest.ssa,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  nir_src_for_ssa(&undef->def));<br>
+  Â  Â  Â  Â  Â  Â  Â continue;<br>
+  Â  Â  Â  Â  Â  }<br>
<br>
-  Â  Â  Â  Â nir_alu_instr *mov = nir_alu_instr_create(state-><wbr>shader,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â nir_op_imov);<br>
-  Â  Â  Â  Â mov->src[0].src = nir_src_for_ssa(<br>
-  Â  Â  Â  Â  Â  nir_phi_builder_value_get_<wbr>block_def(node->pb_value, block));<br>
-  Â  Â  Â  Â for (unsigned i = intrin->num_components; i < 4; i++)<br>
-  Â  Â  Â  Â  Â  mov->src[0].swizzle[i] = 0;<br>
+  Â  Â  Â  Â  Â  if (!node->lower_to_ssa)<br>
+  Â  Â  Â  Â  Â  Â  Â continue;<br>
<br>
-  Â  Â  Â  Â assert(intrin->dest.is_ssa);<br>
+  Â  Â  Â  Â  Â  nir_alu_instr *mov = nir_alu_instr_create(state-><wbr>shader,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  nir_op_imov);<br>
+  Â  Â  Â  Â  Â  mov->src[0].src = nir_src_for_ssa(<br>
+  Â  Â  Â  Â  Â  Â  Â nir_phi_builder_value_get_<wbr>block_def(node->pb_value, block));<br>
+  Â  Â  Â  Â  Â  for (unsigned i = intrin->num_components; i < 4; i++)<br>
+  Â  Â  Â  Â  Â  Â  Â mov->src[0].swizzle[i] = 0;<br>
<br>
-  Â  Â  Â  Â mov->dest.write_mask = (1 << intrin->num_components) - 1;<br>
-  Â  Â  Â  Â nir_ssa_dest_init(&mov->instr, &mov->dest.dest,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â intrin->num_components,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â intrin->dest.ssa.bit_size, NULL);<br>
+  Â  Â  Â  Â  Â  assert(intrin->dest.is_ssa);<br>
<br>
-  Â  Â  Â  Â nir_instr_insert_before(&<wbr>intrin->instr, &mov->instr);<br>
-  Â  Â  Â  Â nir_instr_remove(&intrin-><wbr>instr);<br>
+  Â  Â  Â  Â  Â  mov->dest.write_mask = (1 << intrin->num_components) - 1;<br>
+  Â  Â  Â  Â  Â  nir_ssa_dest_init(&mov->instr, &mov->dest.dest,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  intrin->num_components,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  intrin->dest.ssa.bit_size, NULL);<br>
<br>
-  Â  Â  Â  Â nir_ssa_def_rewrite_uses(&<wbr>intrin->dest.ssa,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  nir_src_for_ssa(&mov->dest.<wbr>dest.ssa));<br>
-  Â  Â  Â  Â break;<br>
-  Â  Â  }<br>
-<br>
-  Â  Â  case nir_intrinsic_store_var: {<br>
-  Â  Â  Â  Â struct deref_node *node =<br>
-  Â  Â  Â  Â  Â  get_deref_node(intrin-><wbr>variables[0], state);<br>
-<br>
-  Â  Â  Â  Â if (node == NULL) {<br>
-  Â  Â  Â  Â  Â  /* Probably an out-of-bounds array store.  That should be a<br>
-  Â  Â  Â  Â  Â  Â * no-op. */<br>
+  Â  Â  Â  Â  Â  nir_instr_insert_before(&<wbr>intrin->instr, &mov->instr);<br>
  Â  Â  Â  Â  Â  Â nir_instr_remove(&intrin-><wbr>instr);<br>
-  Â  Â  Â  Â  Â  continue;<br>
-  Â  Â  Â  Â }<br>
<br>
-  Â  Â  Â  Â if (!node->lower_to_ssa)<br>
-  Â  Â  Â  Â  Â  continue;<br>
-<br>
-  Â  Â  Â  Â assert(intrin->num_components ==<br>
-  Â  Â  Â  Â  Â  Â  Â  glsl_get_vector_elements(node-<wbr>>type));<br>
-<br>
-  Â  Â  Â  Â assert(intrin->src[0].is_ssa);<br>
+  Â  Â  Â  Â  Â  nir_ssa_def_rewrite_uses(&<wbr>intrin->dest.ssa,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â nir_src_for_ssa(&mov->dest.<wbr>dest.ssa));<br>
+  Â  Â  Â  Â  Â  break;<br>
+  Â  Â  Â  Â }<br>
<br>
-  Â  Â  Â  Â nir_ssa_def *new_def;<br>
-  Â  Â  Â  Â b.cursor = nir_before_instr(&intrin-><wbr>instr);<br>
+  Â  Â  Â  Â case nir_intrinsic_store_var: {<br>
+  Â  Â  Â  Â  Â  struct deref_node *node =<br>
+  Â  Â  Â  Â  Â  Â  Â get_deref_node(intrin-><wbr>variables[0], state);<br>
<br>
-  Â  Â  Â  Â unsigned wrmask = nir_intrinsic_write_mask(<wbr>intrin);<br>
-  Â  Â  Â  Â if (wrmask == (1 << intrin->num_components) - 1) {<br>
-  Â  Â  Â  Â  Â  /* Whole variable store - just copy the source.  Note that<br>
-  Â  Â  Â  Â  Â  Â * intrin->num_components and intrin->src[0].ssa->num_<wbr>components<br>
-  Â  Â  Â  Â  Â  Â * may differ.<br>
-  Â  Â  Â  Â  Â  Â */<br>
-  Â  Â  Â  Â  Â  unsigned swiz[4];<br>
-  Â  Â  Â  Â  Â  for (unsigned i = 0; i < 4; i++)<br>
-  Â  Â  Â  Â  Â  Â  Â swiz[i] = i < intrin->num_components ? i : 0;<br>
+  Â  Â  Â  Â  Â  if (node == NULL) {<br>
+  Â  Â  Â  Â  Â  Â  Â /* Probably an out-of-bounds array store.  That should be a<br>
+  Â  Â  Â  Â  Â  Â  Â  * no-op. */<br>
+  Â  Â  Â  Â  Â  Â  Â nir_instr_remove(&intrin-><wbr>instr);<br>
+  Â  Â  Â  Â  Â  Â  Â continue;<br>
+  Â  Â  Â  Â  Â  }<br>
<br>
-  Â  Â  Â  Â  Â  new_def = nir_swizzle(&b, intrin->src[0].ssa, swiz,<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  intrin->num_components, false);<br>
-  Â  Â  Â  Â } else {<br>
-  Â  Â  Â  Â  Â  nir_ssa_def *old_def =<br>
-  Â  Â  Â  Â  Â  Â  Â nir_phi_builder_value_get_<wbr>block_def(node->pb_value, block);<br>
-  Â  Â  Â  Â  Â  /* For writemasked store_var intrinsics, we combine the newly<br>
-  Â  Â  Â  Â  Â  Â * written values with the existing contents of unwritten<br>
-  Â  Â  Â  Â  Â  Â * channels, creating a new SSA value for the whole vector.<br>
-  Â  Â  Â  Â  Â  Â */<br>
-  Â  Â  Â  Â  Â  nir_ssa_def *srcs[4];<br>
-  Â  Â  Â  Â  Â  for (unsigned i = 0; i < intrin->num_components; i++) {<br>
-  Â  Â  Â  Â  Â  Â  Â if (wrmask & (1 << i)) {<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  srcs[i] = nir_channel(&b, intrin->src[0].ssa, i);<br>
-  Â  Â  Â  Â  Â  Â  Â } else {<br>
-  Â  Â  Â  Â  Â  Â  Â  Â  srcs[i] = nir_channel(&b, old_def, i);<br>
+  Â  Â  Â  Â  Â  if (!node->lower_to_ssa)<br>
+  Â  Â  Â  Â  Â  Â  Â continue;<br>
+<br>
+  Â  Â  Â  Â  Â  assert(intrin->num_components ==<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â glsl_get_vector_elements(node-<wbr>>type));<br>
+<br>
+  Â  Â  Â  Â  Â  assert(intrin->src[0].is_ssa);<br>
+<br>
+  Â  Â  Â  Â  Â  nir_ssa_def *new_def;<br>
+  Â  Â  Â  Â  Â  b.cursor = nir_before_instr(&intrin-><wbr>instr);<br>
+<br>
+  Â  Â  Â  Â  Â  unsigned wrmask = nir_intrinsic_write_mask(<wbr>intrin);<br>
+  Â  Â  Â  Â  Â  if (wrmask == (1 << intrin->num_components) - 1) {<br>
+  Â  Â  Â  Â  Â  Â  Â /* Whole variable store - just copy the source.  Note that<br>
+  Â  Â  Â  Â  Â  Â  Â  * intrin->num_components and intrin->src[0].ssa->num_<wbr>components<br>
+  Â  Â  Â  Â  Â  Â  Â  * may differ.<br>
+  Â  Â  Â  Â  Â  Â  Â  */<br>
+  Â  Â  Â  Â  Â  Â  Â unsigned swiz[4];<br>
+  Â  Â  Â  Â  Â  Â  Â for (unsigned i = 0; i < 4; i++)<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  swiz[i] = i < intrin->num_components ? i : 0;<br>
+<br>
+  Â  Â  Â  Â  Â  Â  Â new_def = nir_swizzle(&b, intrin->src[0].ssa, swiz,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â intrin->num_components, false);<br>
+  Â  Â  Â  Â  Â  } else {<br>
+  Â  Â  Â  Â  Â  Â  Â nir_ssa_def *old_def =<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  nir_phi_builder_value_get_<wbr>block_def(node->pb_value, block);<br>
+  Â  Â  Â  Â  Â  Â  Â /* For writemasked store_var intrinsics, we combine the newly<br>
+  Â  Â  Â  Â  Â  Â  Â  * written values with the existing contents of unwritten<br>
+  Â  Â  Â  Â  Â  Â  Â  * channels, creating a new SSA value for the whole vector.<br>
+  Â  Â  Â  Â  Â  Â  Â  */<br>
+  Â  Â  Â  Â  Â  Â  Â nir_ssa_def *srcs[4];<br>
+  Â  Â  Â  Â  Â  Â  Â for (unsigned i = 0; i < intrin->num_components; i++) {<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  if (wrmask & (1 << i)) {<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â srcs[i] = nir_channel(&b, intrin->src[0].ssa, i);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  } else {<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â srcs[i] = nir_channel(&b, old_def, i);<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  }<br>
  Â  Â  Â  Â  Â  Â  Â  }<br>
+  Â  Â  Â  Â  Â  Â  Â new_def = nir_vec(&b, srcs, intrin->num_components);<br>
  Â  Â  Â  Â  Â  Â }<br>
-  Â  Â  Â  Â  Â  new_def = nir_vec(&b, srcs, intrin->num_components);<br>
-  Â  Â  Â  Â }<br>
<br>
-  Â  Â  Â  Â assert(new_def->num_components == intrin->num_components);<br>
+  Â  Â  Â  Â  Â  assert(new_def->num_components == intrin->num_components);<br>
<br>
-  Â  Â  Â  Â nir_phi_builder_value_set_<wbr>block_def(node->pb_value, block, new_def);<br>
-  Â  Â  Â  Â nir_instr_remove(&intrin-><wbr>instr);<br>
-  Â  Â  Â  Â break;<br>
-  Â  Â  }<br>
+  Â  Â  Â  Â  Â  nir_phi_builder_value_set_<wbr>block_def(node->pb_value, block, new_def);<br>
+  Â  Â  Â  Â  Â  nir_instr_remove(&intrin-><wbr>instr);<br>
+  Â  Â  Â  Â  Â  break;<br>
+  Â  Â  Â  Â }<br>
<br>
-  Â  Â  default:<br>
-  Â  Â  Â  Â break;<br>
+  Â  Â  Â  Â default:<br>
+  Â  Â  Â  Â  Â  break;<br>
+  Â  Â  Â  Â }<br>
  Â  Â  Â }<br>
  Â  }<br>
<br>
-  Â for (unsigned i = 0; i < block->num_dom_children; ++i)<br>
-  Â  Â  rename_variables_block(block-><wbr>dom_children[i], state);<br>
-<br>
  Â  return true;<br>
 }<br>
<br>
@@ -737,7 +736,7 @@ nir_lower_vars_to_ssa_impl(<wbr>nir_function_impl *impl)<br>
  Â  Â  Â }<br>
  Â  }<br>
<br>
-  Â rename_variables_block(nir_<wbr>start_block(impl), &state);<br>
+  Â rename_variables(&state);<br>
<br>
  Â  nir_phi_builder_finish(state.<wbr>phi_builder);<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
2.7.3<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>