<p dir="ltr">I had some very minor comments but, with those addressed 1-4 are</p>
<p dir="ltr">Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>></p>
<div class="gmail_quote">On Dec 11, 2015 1:24 PM, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Instead of performing the read-modify-write cycle in glsl->nir, we can<br>
simply emit a partial writemask. For locals, nir_lower_vars_to_ssa will<br>
do the equivalent read-modify-write cycle for us, so we continue to get<br>
the same SSA values we had before.<br>
<br>
Because glsl_to_nir calls nir_lower_outputs_to_temporaries, all outputs<br>
are shadowed with temporary values, and written out as whole vectors at<br>
the end of the shader. So, most consumers will still not see partial<br>
writemasks.<br>
<br>
However, nir_lower_outputs_to_temporaries bails for tessellation control<br>
shader outputs. So those remain actual variables, and stores to those<br>
variables now get a writemask. nir_lower_io passes that through. This<br>
means that TCS outputs should actually work now.<br>
<br>
This is a functional change for tessellation control shaders.<br>
<br>
v2: Relax the nir_validate assert to allow partial writemasks.<br>
<br>
Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
---<br>
 src/glsl/nir/glsl_to_nir.cpp | 39 +++++++++------------------------------<br>
 src/glsl/nir/nir_validate.c | 3 +--<br>
 2 files changed, 10 insertions(+), 32 deletions(-)<br>
<br>
diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp<br>
index 14f3153..a45e2a0 100644<br>
--- a/src/glsl/nir/glsl_to_nir.cpp<br>
+++ b/src/glsl/nir/glsl_to_nir.cpp<br>
@@ -1130,44 +1130,23 @@ nir_visitor::visit(ir_assignment *ir)<br>
  nir_ssa_def *src = evaluate_rvalue(ir->rhs);<br>
<br>
  if (ir->write_mask != (1 << num_components) - 1 && ir->write_mask != 0) {<br>
-Â Â Â /*<br>
-Â Â Â Â * We have no good way to update only part of a variable, so just load<br>
-Â Â Â Â * the LHS and do a vec operation to combine the old with the new, and<br>
-Â Â Â Â * then store it<br>
-Â Â Â Â * back into the LHS. Copy propagation should get rid of the mess.<br>
+Â Â Â /* GLSL IR will give us the input to the write-masked assignment in a<br>
+    * single packed vector. So, for example, if the writemask is xzw, then<br>
+Â Â Â Â * we have to swizzle x -> x, y -> z, and z -> w and get the y component<br>
+Â Â Â Â * from the load.<br>
    */<br>
-<br>
-Â Â Â nir_intrinsic_instr *load =<br>
-Â Â Â Â Â nir_intrinsic_instr_create(this->shader, nir_intrinsic_load_var);<br>
-Â Â Â load->num_components = ir->lhs->type->vector_elements;<br>
-Â Â Â nir_ssa_dest_init(&load->instr, &load->dest, num_components, NULL);<br>
-Â Â Â load->variables[0] = lhs_deref;<br>
-Â Â Â ralloc_steal(load, load->variables[0]);<br>
-Â Â Â nir_builder_instr_insert(&b, &load->instr);<br>
-<br>
-Â Â Â nir_ssa_def *srcs[4];<br>
-<br>
+Â Â Â unsigned swiz[4];<br>
    unsigned component = 0;<br>
-Â Â Â for (unsigned i = 0; i < ir->lhs->type->vector_elements; i++) {<br>
-Â Â Â Â Â if (ir->write_mask & (1 << i)) {<br>
-Â Â Â Â Â Â /* GLSL IR will give us the input to the write-masked assignment<br>
-       * in a single packed vector. So, for example, if the<br>
-Â Â Â Â Â Â Â * writemask is xzw, then we have to swizzle x -> x, y -> z,<br>
-Â Â Â Â Â Â Â * and z -> w and get the y component from the load.<br>
-Â Â Â Â Â Â Â */<br>
-Â Â Â Â Â Â srcs[i] = nir_channel(&b, src, component++);<br>
-Â Â Â Â Â } else {<br>
-Â Â Â Â Â Â srcs[i] = nir_channel(&b, &load->dest.ssa, i);<br>
-Â Â Â Â Â }<br>
+Â Â Â for (unsigned i = 0; i < 4; i++) {<br>
+Â Â Â Â Â swiz[i] = ir->write_mask & (1 << i) ? component++ : 0;<br>
    }<br>
-<br>
-Â Â Â src = nir_vec(&b, srcs, ir->lhs->type->vector_elements);<br>
+Â Â Â src = nir_swizzle(&b, src, swiz, num_components, !supports_ints);<br>
  }<br>
<br>
  nir_intrinsic_instr *store =<br>
    nir_intrinsic_instr_create(this->shader, nir_intrinsic_store_var);<br>
  store->num_components = ir->lhs->type->vector_elements;<br>
-Â Â store->const_index[0] = (1 << store->num_components) - 1;<br>
+Â Â store->const_index[0] = ir->write_mask;<br>
  nir_deref *store_deref = nir_copy_deref(store, &lhs_deref->deref);<br>
  store->variables[0] = nir_deref_as_var(store_deref);<br>
  store->src[0] = nir_src_for_ssa(src);<br>
diff --git a/src/glsl/nir/nir_validate.c b/src/glsl/nir/nir_validate.c<br>
index 89cf0b8..da92055 100644<br>
--- a/src/glsl/nir/nir_validate.c<br>
+++ b/src/glsl/nir/nir_validate.c<br>
@@ -417,8 +417,7 @@ validate_intrinsic_instr(nir_intrinsic_instr *instr, validate_state *state)<br>
    assert(instr->variables[0]->var->data.mode != nir_var_shader_in &&<br>
       instr->variables[0]->var->data.mode != nir_var_uniform &&<br>
       instr->variables[0]->var->data.mode != nir_var_shader_storage);<br>
-Â Â Â /* Currently, writemasks must cover the entire value */<br>
-Â Â Â assert(instr->const_index[0] == (1 << instr->num_components) - 1);<br>
+Â Â Â assert((instr->const_index[0] & ~((1 << instr->num_components) - 1)) == 0);<br>
    break;<br>
  }<br>
  case nir_intrinsic_copy_var:<br>
--<br>
2.6.3<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div>