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

[Timothy Arceri]

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 
> 
> ---
> 
> NOTE: Someone with access will need to commit this after the review
>   process
> 

I've pushed this thanks for fixing it :)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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
 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 
>>
>> ---
>>
>> 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(_variables);
>> - main_func_sig->body.append_list(_instructions);
>> +
>> + lower_packed_varyings_return_splicer splicer(mem_ctx,
>> _instructions);
>> +
>> + main_func_sig->body.head->insert_before(_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(_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

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

[Timothy Arceri]

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 
> 
> ---
> 
> 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(_variables);
> - main_func_sig->body.append_list(_instructions);
> +
> + lower_packed_varyings_return_splicer splicer(mem_ctx,
> _instructions);
> +
> + main_func_sig->body.head->insert_before(_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(_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

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

[Lars Hamre]

Gentle ping, I would appreciate feedback.

Regards,
Lars Hamre

On Sun, Apr 17, 2016 at 1:18 PM, 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 
>
> ---
>
> 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_splicer(
> +  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(_variables);
> - main_func_sig->body.append_list(_instructions);
> +
> + lower_packed_varyings_return_splicer splicer(mem_ctx, 
> _instructions);
> +
> + main_func_sig->body.head->insert_before(_variables);
> +
> + splicer.run(instructions);
> +
> + /* 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(_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

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

[Lars Hamre]

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 

---

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_splicer(
+  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(_variables);
- main_func_sig->body.append_list(_instructions);
+
+ lower_packed_varyings_return_splicer splicer(mem_ctx, 
_instructions);
+
+ main_func_sig->body.head->insert_before(_variables);
+
+ splicer.run(instructions);
+
+ /* 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(_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