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

[Lars Hamre]

Hi Timothy,

Thanks for your comments, you're absolutely right about not wanting to
visit other functions.
I will modify the visitor appropriately and submit a couple of piglit tests.

Regards,
Lars Hamre


On Mon, Apr 25, 2016 at 8:20 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> Hi Lars,
>
> Thankd for working on this, comments below.
>
> On Sun, 2016-04-17 at 13:18 -0400, Lars Hamre wrote:
>> 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>
>>
>> ---
>>
>> NOTE: Someone with access will need to commit this after the review
>>       process
>>
>>  src/compiler/glsl/lower_packed_varyings.cpp | 58
>> +++++++++++++++++++++++++++--
>>  1 file changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_packed_varyings.cpp
>> b/src/compiler/glsl/lower_packed_varyings.cpp
>> index 825cc9e..41edada 100644
>> --- a/src/compiler/glsl/lower_packed_varyings.cpp
>> +++ b/src/compiler/glsl/lower_packed_varyings.cpp
>> @@ -724,6 +724,45 @@
>> lower_packed_varyings_gs_splicer::visit_leave(ir_emit_vertex *ev)
>>     return visit_continue;
>>  }
>>
>> +/**
>> + * Visitor that splices varying packing code before every return.
>> + */
>> +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);
>> +
>> +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;
>> +}
>>
>>  void
>>  lower_packed_varyings(void *mem_ctx, unsigned locations_used,
>> @@ -757,11 +796,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()
>>            */
>> -         main_func_sig->body.append_list(&new_variables);
>> -         main_func_sig->body.append_list(&new_instructions);
>> +
>> +         lower_packed_varyings_return_splicer splicer(mem_ctx,
>> &new_instructions);
>> +
>> +         main_func_sig->body.head->insert_before(&new_variables);
>> +
>> +         splicer.run(instructions);
>
> This will walk over everything. I don't think you want that you only
> want to walk over the instructions in main right?
>
> For example if you have another function with a return then you would
> end up inserting the instructions before that functions return too. You
> could probably create a piglit test to detect this.
>
>
>> +
>> +         /* Lower outputs at the end of main() if the last
>> instruction is not
>> +          * a return statement
>> +          */
>> +         if (((ir_instruction*)instructions->get_tail())->ir_type !=
>> ir_type_return) {
>> +            main_func_sig->body.append_list(&new_instructions);
>> +         }
>
> Will this work for:
>
>
>         foo1 = vec2(0.5);
>         if (early_return != 0) {
>                 foo1 = vec2(0.2);
>                 return;
>         }
>
> I assume it will be fine, but would be good to have a piglit test for
> this also.
>
>>        }
>>     } else {
>>        /* Shader inputs need to be lowered at the beginning of main()
>> */
>> --
>> 2.5.5
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev