Re: [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
 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 
>>
>> ---
>>
>> CC: Timothy Arceri 
>>
>> 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 

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

[Timothy Arceri]

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 
> 
> ---
> 
> CC: Timothy Arceri 
> 
> 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 {
> - /* 

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

[Lars Hamre]

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 

---

CC: Timothy Arceri 

Hi Timothy,

As it turns out, functions are inlined prior to
lower_packed_varyings() being called. I put in the check
to not visit other functions anyways in case that changes
at some point in the future.

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.

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.

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_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;
+}
+
+ir_visitor_status
+lower_packed_varyings_return_splicer::visit_enter(ir_function_signature *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, 
_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
   */
- main_func_sig->body.append_list(_variables);
- main_func_sig->body.append_list(_instructions);
+ 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