On 07/05/2011 03:07 PM, Paul Berry wrote:
The visitor class in lower_jumps.cpp never removes or replaces the
instruction being visited, but it frequently alters or removes the
instructions that follow it. Therefore, in order to be safe, it needs
to iterate through exec_lists using foreach_list rather than
visit_exec_list().
Not technically "to be safe", but so it actually visits the newly
created IR...
Without this patch, lower_jumps.cpp may require multiple passes in
order to lower all jumps, resulting in sub-optimal output. Also,
certain invariants assumed by lower_jumps.cpp may fail to hold,
causing assertion failures.
...well, unless this is true, in which case, yes, safety :)
Definitely sub-optimal since it'll likely need /several/ passes, each of
which would have to generate new temporaries for execution-guarding,
return values, and so on.
Regardless, this is definitely the right thing to do, so:
Reviewed-by: Kenneth Graunke <kenn...@whitecape.org>
Fixes unit tests test_lower_pulled_out_jump,
test_lower_unified_returns, test_lower_guarded_conditional_break,
test_lower_return_non_void_at_end_of_loop, and test_lower_returns_3.
---
src/glsl/lower_jumps.cpp | 13 ++++++++++++-
1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/src/glsl/lower_jumps.cpp b/src/glsl/lower_jumps.cpp
index 51a4cf0..103d64f 100644
--- a/src/glsl/lower_jumps.cpp
+++ b/src/glsl/lower_jumps.cpp
@@ -447,9 +447,20 @@ struct ir_lower_jumps_visitor : public
ir_control_flow_visitor {
block_record visit_block(exec_list* list)
{
+ /* Note: since visiting a node may change that node's next
+ * pointer, we can't use visit_exec_list(), because
+ * visit_exec_list() caches the node's next pointer before
+ * visiting it. So we use foreach_list() instead.
+ *
+ * foreach_list() isn't safe if the node being visited gets
+ * removed, but fortunately this visitor doesn't do that.
+ */
+
block_record saved_block = this->block;
this->block = block_record();
- visit_exec_list(list, this);
+ foreach_list(node, list) {
+ ((ir_instruction *) node)->accept(this);
+ }
block_record ret = this->block;
this->block = saved_block;
return ret;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
