Mesa (master): glsl: Don' t trust loop analysis in the presence of function calls.

[Kenneth Graunke]

Module: Mesa
Branch: master
Commit: 0405bd08ca0e01ebc68891ee1ff47d320983f775
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=0405bd08ca0e01ebc68891ee1ff47d320983f775

Author: Kenneth Graunke <kenneth at whitecape.org>
Date:   Wed Mar 28 14:00:42 2012 -0700

glsl: Don't trust loop analysis in the presence of function calls.

Function calls may have side effects that alter variables used inside
the loop.  In the fragment shader, they may even terminate the shader.
This means our analysis about loop-constant or induction variables may
be completely wrong.

In general it's impossible to determine whether they actually do or not
(due to the halting problem), so we'd need to perform conservative
static analysis.  For now, it's not worth the complexity: most functions
will be inlined, at which point we can unroll them successfully.

Fixes Piglit tests:
- shaders/glsl-fs-unroll-out-param
- shaders/glsl-fs-unroll-side-effect

NOTE: This is a candidate for release branches.

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

---

 src/glsl/loop_analysis.cpp |   28 ++++++++++++++++++++++++++++
 src/glsl/loop_analysis.h   |    6 ++++++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/src/glsl/loop_analysis.cpp b/src/glsl/loop_analysis.cpp
index 9bba6a9..6a0e4da 100644
--- a/src/glsl/loop_analysis.cpp
+++ b/src/glsl/loop_analysis.cpp
@@ -110,6 +110,8 @@ public:
    virtual ir_visitor_status visit(ir_loop_jump *);
    virtual ir_visitor_status visit(ir_dereference_variable *);
 
+   virtual ir_visitor_status visit_enter(ir_call *);
+
    virtual ir_visitor_status visit_enter(ir_loop *);
    virtual ir_visitor_status visit_leave(ir_loop *);
    virtual ir_visitor_status visit_enter(ir_assignment *);
@@ -153,6 +155,21 @@ loop_analysis::visit(ir_loop_jump *ir)
 
 
 ir_visitor_status
+loop_analysis::visit_enter(ir_call *ir)
+{
+   /* If we're not somewhere inside a loop, there's nothing to do. */
+   if (this->state.is_empty())
+      return visit_continue;
+
+   loop_variable_state *const ls =
+      (loop_variable_state *) this->state.get_head();
+
+   ls->contains_calls = true;
+   return visit_continue_with_parent;
+}
+
+
+ir_visitor_status
 loop_analysis::visit(ir_dereference_variable *ir)
 {
    /* If we're not somewhere inside a loop, there's nothing to do.
@@ -209,6 +226,17 @@ loop_analysis::visit_leave(ir_loop *ir)
    loop_variable_state *const ls =
       (loop_variable_state *) this->state.pop_head();
 
+   /* Function calls may contain side effects.  These could alter any of our
+    * variables in ways that cannot be known, and may even terminate shader
+    * execution (say, calling discard in the fragment shader).  So we can't
+    * rely on any of our analysis about assignments to variables.
+    *
+    * We could perform some conservative analysis (prove there's no statically
+    * possible assignment, etc.) but it isn't worth it for now; function
+    * inlining will allow us to unroll loops anyway.
+    */
+   if (ls->contains_calls)
+      return visit_continue;
 
    foreach_list(node, &ir->body_instructions) {
       /* Skip over declarations at the start of a loop.
diff --git a/src/glsl/loop_analysis.h b/src/glsl/loop_analysis.h
index 2297308..8bed1db 100644
--- a/src/glsl/loop_analysis.h
+++ b/src/glsl/loop_analysis.h
@@ -122,10 +122,16 @@ public:
     */
    unsigned num_loop_jumps;
 
+   /**
+    * Whether this loop contains any function calls.
+    */
+   bool contains_calls;
+
    loop_variable_state()
    {
       this->max_iterations = -1;
       this->num_loop_jumps = 0;
+      this->contains_calls = false;
       this->var_hash = hash_table_ctor(0, hash_table_pointer_hash,
 				       hash_table_pointer_compare);
    }