[Mesa-dev] [PATCH v2] glsl: fix lowering outputs for early/nested returns

[Timothy Arceri]

On Tue, 2016-04-26 at 19:50 -0400, Lars Hamre wrote:
> v2: limit lowerings to return statments in main()
> 
> Return statements in conditional blocks were not having their
> output varyings lowered correctly.
> 
> This patch fixes the following piglit tests:
> /spec/glsl-1.10/execution/vs-float-main-return
> /spec/glsl-1.10/execution/vs-vec2-main-return
> /spec/glsl-1.10/execution/vs-vec3-main-return
> 
> Signed-off-by: Lars Hamre <chemecse at gmail.com>
> 
> ---
> 
> CC: Timothy Arceri <timothy.arceri at collabora.com>
> 
> Hi Timothy,
> 
> As it turns out, functions are inlined prior to
> lower_packed_varyings() being called.

Ok thanks for checking.

>  I put in the check
> to not visit other functions anyways in case that changes
> at some point in the future.

We can probably just go with V1. I'm not really a fan of extra code
that doesn't get used.

> 
> I could not determine a way to test having the splicer
> inserting instructions into a function that is not main()
> within piglit. Inserting the lowering instructions before
> a function's return statement just inserts "useless"
> instructions, but does not produce incorrect results.
> Please nudge me in the right direction if you have an idea.

I had convinced myself yesterday it could be tested but your right the
values should just be overwritten again before we can ever access them.
Guess I should wait til I've woken up a bit more in the morning before
reviewing patches :P

> 
> There already exists piglit tests which check float, vec2,
> and vec3 varyings for early returns (which this patch
> fixes). Let me know if you have anything else to test for
> in mind.

The test I suggested in my review is slightly different from the
existing tests:

	foo1 = vec2(0.5);
	if (early_return != 0) {
		foo1 = vec2(0.2);
		return;
	}

vs

	foo1 = vec2(0.5);
	if (early_return != 0)
		return;
	foo1 = vec2(0.2);

I think your code should handle it ok but it would be nice to test this
alternative also. My concern was that the last instruction is now the
return so the copy instructions might not get added, although I think
it should be ok as your code should only see the if.

The other thing that would be nice is to update the existing tests to
also check the alternative path. So after the existing probe you would
add 'uniform int early_return 0' and test for 0.2.

As for this patch I'll add my R-B to the first version and push it in a
little bit if there is no further feedback.

> 
> Regards,
> Lars Hamre
> 
> NOTE: Someone with access will need to commit this after the review
>       process
> 
>  src/compiler/glsl/lower_packed_varyings.cpp | 68
> +++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 4 deletions(-)
> 
> diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
> b/src/compiler/glsl/lower_packed_varyings.cpp
> index 825cc9e..c9197b7 100644
> --- a/src/compiler/glsl/lower_packed_varyings.cpp
> +++ b/src/compiler/glsl/lower_packed_varyings.cpp
> @@ -724,6 +724,55 @@
> lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
>     return visit_continue;
>  }
> 
> +/**
> + * Visitor that splices varying packing code before every return in
> main().
> + */
> +class lower_packed_varyings_return_splicer : public
> ir_hierarchical_visitor
> +{
> +public:
> +   explicit lower_packed_varyings_return_splicer(void *mem_ctx,
> +                                                 const exec_list
> *instructions);
> +
> +   virtual ir_visitor_status visit_leave(ir_return *ret);
> +   virtual ir_visitor_status visit_enter(ir_function_signature
> *sig);
> +
> +private:
> +   /**
> +    * Memory context used to allocate new instructions for the
> shader.
> +    */
> +   void * const mem_ctx;
> +
> +   /**
> +    * Instructions that should be spliced into place before each
> return.
> +    */
> +   const exec_list *instructions;
> +};
> +
> +
> +lower_packed_varyings_return_splicer::lower_packed_varyings_return_s
> plicer(
> +      void *mem_ctx, const exec_list *instructions)
> +   : mem_ctx(mem_ctx), instructions(instructions)
> +{
> +}
> +
> +ir_visitor_status
> +lower_packed_varyings_return_splicer::visit_leave(ir_return *ret)
> +{
> +   foreach_in_list(ir_instruction, ir, this->instructions) {
> +      ret->insert_before(ir->clone(this->mem_ctx, NULL));
> +   }
> +   return visit_continue;
> +}
> +
> +ir_visitor_status
> +lower_packed_varyings_return_splicer::visit_enter(ir_function_signat
> ure *sig)
> +{
> +   /* Only splice returns into main(). */
> +   if (!strcmp(sig->function_name(), "main"))
> +      return visit_continue;
> +   return visit_continue_with_parent;
> +}
> +
> 
>  void
>  lower_packed_varyings(void *mem_ctx, unsigned locations_used,
> @@ -757,11 +806,22 @@ lower_packed_varyings(void *mem_ctx, unsigned
> locations_used,
>           /* Now update all the EmitVertex instances */
>           splicer.run(instructions);
>        } else {
> -         /* For other shader types, outputs need to be lowered at
> the end of
> -          * main()
> +         /* For other shader types, outputs need to be lowered
> before each
> +          * return statement and at the end of main()
> +          */
> +
> +         lower_packed_varyings_return_splicer splicer(mem_ctx,
> &new_instructions);
> +
> +         main_func_sig->body.head->insert_before(&new_variables);
> +
> +         splicer.run(instructions);
> +
> +         /* Lower outputs at the end of main() if the last
> instruction is not
> +          * a return statement
>            */
> -         main_func_sig->body.append_list(&new_variables);
> -         main_func_sig->body.append_list(&new_instructions);
> +         if (((ir_instruction*)instructions->get_tail())->ir_type !=
> ir_type_return) {
> +            main_func_sig->body.append_list(&new_instructions);
> +         }
>        }
>     } else {
>        /* Shader inputs need to be lowered at the beginning of main()
> */
> --
> 2.5.5
>