Thanks again, I will make/modify those piglit tests.
Regards,
Lars Hamre
On Tue, Apr 26, 2016 at 9:20 PM, Timothy Arceri
<timothy.arc...@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 <cheme...@gmail.com>
>>
>> ---
>>
>> CC: Timothy Arceri <timothy.arc...@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
>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
