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

[Lars Hamre]

Thanks again, I will make/modify those piglit tests.

Regards,
Lars Hamre

On Tue, Apr 26, 2016 at 9:20 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> 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
>>