<div dir="ltr">On 15 September 2013 00:10, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">And fix the dead code elimination pass so atomic writes aren't<br>
optimized out in cases where the return value isn't used by the<br>
program.<br></blockquote><div><br></div><div>As I mentioned in my comments on patch 9, I'd prefer if we went with a different approach where we don't add ir_rvalue nodes that have side effects at all, but instead stick with our current system where only ir_instruction nodes can have side effects.<br>
<br>However, if we do wind up going with this approach, we'll need to make has_side_effects() recurse through the expression tree, so that it can see that expressions like (atomicCounterIncrement(c) + 1) and foo[atomicCounterIncrement(c)] also have side effects.<br>
<br></div><div>We would also need to modify several other optimization passes to check for side effects:<br><br></div><div>- opt_dead_code_local (which does the same job as opt_dead_code, but within basic blocks)<br><br></div>
<div>- opt_tree_grafting - as is, this would try to transform:<br><br></div><div>uint x = atomicCounterIncrement(c);<br></div><div>uint y = atomicCounterIncrement(c);<br></div><div>uvec3 v = uvec3(y, x, y);<br><br>Into:<br>
<br></div><div>uint y = atomicCounterIncrement(c);<br></div><div>uvec3 v = uvec3(y, atomicCounterIncrement(c), y);<br><br>Which is not equivalent.<br><br></div><div>- lower_variable_index_to_cond_assign, lower_vec_index_to_cond_assign, and lower_vector_insert (which sometimes clone arbitrary rvalues)<br>
<br></div><div>And possibly others I've missed.<br></div><div><br></div><div><br></div><div>There's also at least one contrived case which is propably not worth fixing:<br></div><div><br></div><div>- opt_flip_matrices (in principle this might try to rewrite gl_TextureMatrix[atomicCounterIncrement(c)] * foo[atomicCounterIncrement(c)] to foo[atomicCounterIncrement(c)] * gl_TextureMatrixTranspose(c), which is technically not the same operation).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
---<br>
 src/glsl/ir.h              | 16 ++++++++++++++++<br>
 src/glsl/opt_dead_code.cpp |  3 ++-<br>
 2 files changed, 18 insertions(+), 1 deletion(-)<br>
<br>
diff --git a/src/glsl/ir.h b/src/glsl/ir.h<br>
index c4b4677..4f506a3 100644<br>
--- a/src/glsl/ir.h<br>
+++ b/src/glsl/ir.h<br>
@@ -139,6 +139,17 @@ public:<br>
    virtual class ir_jump *              as_jump()             { return NULL; }<br>
    /*@}*/<br>
<br>
+   /**<br>
+    * Determine if an IR instruction has side effects other than its<br>
+    * returned value(s).  Optimization passes are expected to be<br>
+    * especially careful with reordering or removing these, unless<br>
+    * they know what they are doing.<br>
+    */<br>
+   virtual bool has_side_effects() const<br>
+   {<br>
+      return false;<br>
+   }<br>
+<br>
 protected:<br>
    ir_instruction()<br>
    {<br>
@@ -2120,6 +2131,11 @@ public:<br>
<br>
    virtual ir_visitor_status accept(ir_hierarchical_visitor *);<br>
<br>
+   virtual bool has_side_effects() const<br>
+   {<br>
+      return true;<br>
+   }<br>
+<br>
    /** Kind of atomic instruction. */<br>
    enum ir_atomic_opcode op;<br>
<br>
diff --git a/src/glsl/opt_dead_code.cpp b/src/glsl/opt_dead_code.cpp<br>
index b65e5c2..fd05034 100644<br>
--- a/src/glsl/opt_dead_code.cpp<br>
+++ b/src/glsl/opt_dead_code.cpp<br>
@@ -81,7 +81,8 @@ do_dead_code(exec_list *instructions, bool uniform_locations_assigned)<br>
          */<br>
         if (entry->var->mode != ir_var_function_out &&<br>
             entry->var->mode != ir_var_function_inout &&<br>
-             entry->var->mode != ir_var_shader_out) {<br>
+             entry->var->mode != ir_var_shader_out &&<br>
+             !entry->assign->rhs->has_side_effects()) {<br>
            entry->assign->remove();<br>
            progress = true;<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
1.8.3.4<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" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>