From 0405bd08ca0e01ebc68891ee1ff47d320983f775 Mon Sep 17 00:00:00 2001 From: Kenneth Graunke Date: Wed, 28 Mar 2012 14:00:42 -0700 Subject: [PATCH] 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 Reviewed-by: Ian Romanick --- src/glsl/loop_analysis.cpp | 28 ++++++++++++++++++++++++++++ src/glsl/loop_analysis.h | 6 ++++++ 2 files changed, 34 insertions(+) diff --git a/src/glsl/loop_analysis.cpp b/src/glsl/loop_analysis.cpp index 9bba6a97c48..6a0e4da5100 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 *); @@ -152,6 +154,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) { @@ -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 229730836a8..8bed1db0210 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); } -- 2.30.2