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.arc...@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 <cheme...@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
