[Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
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(&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); + + /* 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); + } } } 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
Re: [Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
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(&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); > + > + /* 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); > + } >} > } 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
Re: [Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
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(&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
Re: [Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
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(&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
Re: [Mesa-dev] [PATCH] glsl: fix lowering outputs for early/nested returns
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