[Mesa-dev] [PATCH 09/11] glsl: Use foreach_list in lower_jumps.cpp

[Kenneth Graunke]

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 <kenneth at 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;