On Fri, Jan 31, 2014 at 11:55 AM, Paul Berry <stereotype...@gmail.com>wrote:
> On 22 January 2014 09:16, Connor Abbott <cwabbo...@gmail.com> wrote:
>
>> Right now we are being basically as naive as possible, and inserting
>> more copies than necessary. It is possible to implement a more
>> sophisticated algorithm later, although extending the current copy
>> propagation pass to support loops better and/or relying on backends to
>> do copy propagation may make this unecessary.
>> ---
>> src/glsl/Makefile.sources | 1 +
>> src/glsl/ir_optimization.h | 1 +
>> src/glsl/opt_from_ssa.cpp | 198
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 200 insertions(+)
>> create mode 100644 src/glsl/opt_from_ssa.cpp
>>
>> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
>> index 961784b..55859ed 100644
>> --- a/src/glsl/Makefile.sources
>> +++ b/src/glsl/Makefile.sources
>> @@ -94,6 +94,7 @@ LIBGLSL_FILES = \
>> $(GLSL_SRCDIR)/opt_dead_functions.cpp \
>> $(GLSL_SRCDIR)/opt_flatten_nested_if_blocks.cpp \
>> $(GLSL_SRCDIR)/opt_flip_matrices.cpp \
>> + $(GLSL_SRCDIR)/opt_from_ssa.cpp \
>> $(GLSL_SRCDIR)/opt_function_inlining.cpp \
>> $(GLSL_SRCDIR)/opt_if_simplification.cpp \
>> $(GLSL_SRCDIR)/opt_noop_swizzle.cpp \
>> diff --git a/src/glsl/ir_optimization.h b/src/glsl/ir_optimization.h
>> index 92c8b57..9c0ff31 100644
>> --- a/src/glsl/ir_optimization.h
>> +++ b/src/glsl/ir_optimization.h
>> @@ -66,6 +66,7 @@ enum lower_packing_builtins_op {
>> };
>>
>> void convert_to_ssa(exec_list *instructions);
>> +void convert_from_ssa(exec_list *instructions);
>>
>> bool do_common_optimization(exec_list *ir, bool linked,
>> bool uniform_locations_assigned,
>> diff --git a/src/glsl/opt_from_ssa.cpp b/src/glsl/opt_from_ssa.cpp
>> new file mode 100644
>> index 0000000..6071c45
>> --- /dev/null
>> +++ b/src/glsl/opt_from_ssa.cpp
>> @@ -0,0 +1,198 @@
>> +/*
>> + * Copyright © 2013 Connor Abbott (con...@abbott.cx)
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining
>> a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including the
>> next
>> + * paragraph) shall be included in all copies or substantial portions of
>> the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
>> + * DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include "ir.h"
>> +#include "ir_builder.h"
>> +
>> +/**
>> + * \file opt_from_ssa.cpp
>> + *
>> + * This file removes all the SSA temporaries and phi nodes from a
>> program. It
>> + * immplements Method I of the paper "Translating out of Single Static
>> + * Assignment Form" by Sreedhar et. al., a naive method that inserts
>> many more
>> + * copies than necessary; it is assumed that later copy propagation
>> passes will
>> + * clean up the result of this pass.
>> + */
>> +
>> +using namespace ir_builder;
>> +
>> +static ir_variable *
>> +insert_decl(exec_list *instrs, const glsl_type *type, void *mem_ctx)
>> +{
>> + ir_variable *var = new(mem_ctx) ir_variable(type, "phi_temp",
>> + ir_var_temporary);
>> + instrs->push_head(var);
>> + return var;
>> +}
>> +
>> +static void
>> +eliminate_phi_if(ir_phi_if *phi, ir_if *ir, exec_list *instrs)
>> +{
>> + ir_variable *var = insert_decl(instrs, phi->dest->type,
>> ralloc_parent(ir));
>> +
>> + /*
>> + * This converts the destination of the phi node into a non-SSA
>> variable,
>> + * which ir_from_ssa_visitor::visit(ir_dereference_variable *) would
>> normally
>> + * do. We need to do this here because otherwise, the assignment we're
>> + * inserting here will get skipped by the list visitor macro and it
>> won't
>> + * get converted.
>> + */
>>
>
> I found this comment confusing because the use of "otherwise" seems to
> imply that what we're doing here will cause the list visitor to visit the
> new node; in fact what is happening is that we're compensating for the fact
> that it won't. How about instead saying something like "We need to do this
> here because the list visitor uses foreach_list_safe(), so it will skip any
> nodes we insert."
>
>
>> +
>> + ir->insert_after(phi->dest);
>> + phi->dest->insert_after(assign(phi->dest, var));
>> + phi->dest->data.mode = ir_var_temporary;
>>
> +
>> + if (phi->if_src != NULL)
>> + ir->then_instructions.push_tail(assign(var, phi->if_src));
>> +
>> + if (phi->else_src != NULL)
>> + ir->else_instructions.push_tail(assign(var, phi->else_src));
>> +
>> + phi->remove();
>> +}
>> +
>> +static void
>> +eliminate_phi_loop_begin(ir_phi_loop_begin *phi, ir_loop *ir, exec_list
>> *instrs)
>> +{
>> + ir_variable *var = insert_decl(instrs, phi->dest->type,
>> ralloc_parent(ir));
>> + ir->body_instructions.push_head(phi->dest);
>> + phi->dest->insert_after(assign(phi->dest, var));
>> + phi->dest->data.mode = ir_var_temporary;
>> +
>> + if (phi->enter_src != NULL)
>> + ir->insert_before(assign(var, phi->enter_src));
>> +
>> + if (phi->repeat_src != NULL)
>> + ir->body_instructions.push_tail(assign(var, phi->repeat_src));
>> +
>> + foreach_list(n, &phi->continue_srcs) {
>> + ir_phi_jump_src *src = (ir_phi_jump_src *) n;
>> +
>> + if (src->src != NULL)
>> + src->jump->insert_before(assign(var, src->src));
>> + }
>> +
>> + phi->remove();
>> +}
>> +
>> +static void
>> +eliminate_phi_loop_end(ir_phi_loop_end *phi, ir_loop *ir, exec_list
>> *instrs)
>> +{
>> + ir_variable *var = insert_decl(instrs, phi->dest->type,
>> ralloc_parent(ir));
>> + ir->insert_after(phi->dest);
>> + phi->dest->insert_after(assign(phi->dest, var));
>> + phi->dest->data.mode = ir_var_temporary;
>> +
>> + foreach_list(n, &phi->break_srcs) {
>> + ir_phi_jump_src *src = (ir_phi_jump_src *) n;
>> +
>> + if (src->src != NULL)
>> + src->jump->insert_before(assign(var, src->src));
>> + }
>> +
>> + phi->remove();
>> +}
>> +
>> +namespace {
>> +
>> +class ir_from_ssa_visitor : public ir_hierarchical_visitor
>> +{
>> +public:
>> + ir_from_ssa_visitor(exec_list *base_instrs) : base_instrs(base_instrs)
>> + {
>> + }
>> +
>> + virtual ir_visitor_status visit_leave(ir_if *);
>> + virtual ir_visitor_status visit_enter(ir_loop *);
>> + virtual ir_visitor_status visit_leave(ir_loop *);
>> + virtual ir_visitor_status visit(ir_dereference_variable *);
>> +
>> +private:
>> + exec_list *base_instrs;
>> +};
>> +
>> +};
>> +
>> +ir_visitor_status
>> +ir_from_ssa_visitor::visit_leave(ir_if *ir)
>> +{
>> + foreach_list_safe(n, &ir->phi_nodes) {
>> + eliminate_phi_if((ir_phi_if *) n, ir, this->base_instrs);
>> + }
>> +
>> + return visit_continue;
>> +}
>> +
>> +ir_visitor_status
>> +ir_from_ssa_visitor::visit_enter(ir_loop *ir)
>> +{
>> + foreach_list_safe(n, &ir->begin_phi_nodes) {
>> + eliminate_phi_loop_begin((ir_phi_loop_begin *) n, ir,
>> this->base_instrs);
>> + }
>> +
>> + return visit_continue;
>> +}
>> +
>> +ir_visitor_status
>> +ir_from_ssa_visitor::visit_leave(ir_loop *ir)
>> +{
>> + foreach_list_safe(n, &ir->end_phi_nodes) {
>> + eliminate_phi_loop_end((ir_phi_loop_end *) n, ir,
>> this->base_instrs);
>> + }
>> +
>> + return visit_continue;
>> +}
>> +
>> +ir_visitor_status
>> +ir_from_ssa_visitor::visit(ir_dereference_variable *ir)
>> +{
>> + if (this->in_assignee && ir->var->data.mode == ir_var_temporary_ssa) {
>> + this->base_ir->insert_before(ir->var);
>> + ir->var->data.mode = ir_var_temporary;
>> + }
>> +
>> + return visit_continue;
>> +}
>> +
>> +static void
>> +convert_from_ssa_function(exec_list *instructions)
>> +{
>> + ir_from_ssa_visitor v(instructions);
>> + v.run(instructions);
>> +}
>> +
>> +void
>> +convert_from_ssa(exec_list *instructions)
>> +{
>> + foreach_list(node, instructions) {
>> + ir_instruction *ir = (ir_instruction *) node;
>> + ir_function *f = ir->as_function();
>> + if (f) {
>> + foreach_list(sig_node, &f->signatures) {
>> + ir_function_signature *sig = (ir_function_signature *)
>> sig_node;
>> +
>> + convert_from_ssa_function(&sig->body);
>> + }
>> + }
>> + }
>> +}
>>
>
> It looks like the only reason you need to manually walk through the
> functions and signatures is so that you can make sure that
> ir_from_ssa_visitor::base_instrs always points to the correct function's
> instruction list. Normally the way we achieve that sort of thing is to do:
>
> ir_visitor_status
> ir_from_ssa_visitor::visit_enter(ir_function_signature *ir)
> {
> this->base_instrs = &ir->body;
> return visit_continue;
> }
>
> ir_visitor_status
> ir_from_ssa_visitor::visit_leave(ir_function_signature *ir)
> {
> this->base_instrs = NULL;
> return visit_continue;
> }
>
> That way you could get rid of convert_from_ssa_function() and
> convert_from_ssa() would just become:
>
> void
> convert_from_ssa(exec_list *instructions)
> {
> ir_from_ssa_visitor v;
> v.run(instructions);
> }
>
OK... I think I got this code by copy-pasting from opt_to_ssa.cpp (which I
got from local_dce.cpp IIRC), where the intention was to avoid processing
global statements; it doesn't matter if we process global statements here,
though, since we won't do anything.
>
>
> I don't have really strong feelings about it, though. Provided that the
> confusing comment in eliminate_phi_if() is fixed, this patch is:
>
> Reviewed-by: Paul Berry <stereotype...@gmail.com>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
