On Thu, 14 Mar 2013, Richard Biener wrote:
> On Wed, 13 Mar 2013, Diego Novillo wrote:
>
> > This patch adds an initial implementation for a new helper type for
> > generating GIMPLE statements.
> >
> > The type is called gimple_builder. There are two main variants:
> > gimple_builder_normal and gimple_builder_ssa. The difference between
> > the two is the temporaries they create. The 'normal' builder creates
> > temporaries in normal form (i.e., VAR_DECLs). The 'ssa' builder
> > creates SSA names.
> >
> > The basic functionality is described in
> > http://gcc.gnu.org/wiki/cxx-conversion/gimple-generation. I expect it
> > to evolve as I address feedback on this initial implementation.
> >
> > The patch implements the initial builder. It has enough functionality
> > to simplify the generation of 3 address assignments (the bulk of all
> > generated code).
> >
> > To use the builder:
> >
> > 1- Declare an instance 'gb' of gimple_builder_normal or
> > gimple_builder_ssa. E.g., gimple_builder_ssa gb;
> >
> > 2- Use gb.add_* to add a new statement to it. This
> > returns an SSA name or VAR_DECL with the value of the added
> > statement.
> >
> > 3- Call gb.insert_*() to insert the sequence of statements in the
> > builder into a statement iterator.
> >
> > For instance, in asan.c we generate the expression:
> >
> > (shadow != 0) & (base_addr & 7) + (size_in_bytes - 1)) >= shadow).
> >
> > with the following code:
> >
> > -----------------------------------------------------------------------------
> > gimple_builder_ssa gb(location);
> > t = gb.add (NE_EXPR, shadow, 0);
> > tree t1 = gb.add (BIT_AND_EXPR, base_addr, 7);
> > t1 = gb.add_type_cast (shadow_type, t1);
> > if (size_in_bytes > 1)
> > t1 = gb.add (PLUS_EXPR, t1, size_in_bytes - 1);
> > t1 = gb.add (GE_EXPR, t1, shadow);
> > t = gb.add (BIT_AND_EXPR, t, t1);
> > gb.insert_after (&gsi, GSI_NEW_STMT);
> > -----------------------------------------------------------------------------
> >
> >
> > In contrast, the original code needed to generate the same expression
> > is significantly longer:
>
> May I propose instead to first (we'll then see whether the facility
> you propose still makes sense) beat some C++ sense into the
> existing gimple-assign building? Like using overloading to be able
> to say
>
> g = gimple_build_assign (gsi, NE_EXPR, auto (), shadow, 0);
> g2 = gimple_build_assign (gsi, BIT_AND_EXPR, auto (), base_addr, 7);
> g = gimple_build_assign (gsi, NOP_EXPR, auto (), g2);
>
> ? Or to get static number of argument checking make it a template on the
> operation code.
>
> That is, try to do as much as you do with your facility with the
> core interface first.
>
> Please.
>
> Instead of using special objects to select an overload that creates
> a new SSA name that could be done with overloading on the number of
> arguments, too. Instead of passing a gsi you could pass a sequence, too,
> or the stmt to append after.
>
> Note that all the automatic type inference you do, like for
>
> t1 = gb.add (PLUS_EXPR, t1, size_in_bytes - 1)
>
> is of course a recipie for problems.
That said - can we please pass in the type of the operation (and thus
the newly created result temporary) _explicitely_ please? Thus
gimple_builder_ssa gb(location);
t = gb.add (NE_EXPR, boolean_type_node, shadow, 0);
tree t1 = gb.add (BIT_AND_EXPR, TREE_TYPE (base_addr), base_addr,
7);
t1 = gb.add_type_cast (shadow_type, t1);
if (size_in_bytes > 1)
t1 = gb.add (PLUS_EXPR, TREE_TYPE (t1), t1, size_in_bytes - 1);
t1 = gb.add (GE_EXPR, boolean_type_node, t1, shadow);
t = gb.add (BIT_AND_EXPR, boolean_type_node, t, t1);
gb.insert_after (&gsi, GSI_NEW_STMT);
we are writing a compiler after all, not some web javascript code.
Richard.
> That's why I would be very much more comfortable with seeing
> incremental improvements to the existing interface (and features
> added) than a whole new facility that is not very much used
> which just dumps a whole lot of new "features" on us.
>
> Thanks,
> Richard.
>
>
> > -----------------------------------------------------------------------------
> > g = gimple_build_assign_with_ops (NE_EXPR,
> > make_ssa_name (boolean_type_node,
> > NULL),
> > shadow,
> > build_int_cst (shadow_type, 0));
> > gimple_set_location (g, location);
> > gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > t = gimple_assign_lhs (g);
> >
> > g = gimple_build_assign_with_ops (BIT_AND_EXPR,
> > make_ssa_name (uintptr_type,
> > NULL),
> > base_addr,
> > build_int_cst (uintptr_type, 7));
> > gimple_set_location (g, location);
> > gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> >
> > g = gimple_build_assign_with_ops (NOP_EXPR,
> > make_ssa_name (shadow_type,
> > NULL),
> > gimple_assign_lhs (g), NULL_TREE);
> > gimple_set_location (g, location);
> > gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > if (size_in_bytes > 1)
> > {
> > g = gimple_build_assign_with_ops (PLUS_EXPR,
> > make_ssa_name (shadow_type,
> > NULL),
> > gimple_assign_lhs (g),
> > build_int_cst (shadow_type,
> > size_in_bytes - 1));
> > gimple_set_location (g, location);
> > gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > }
> >
> > g = gimple_build_assign_with_ops (GE_EXPR,
> > make_ssa_name (boolean_type_node,
> > NULL),
> > gimple_assign_lhs (g),
> > shadow);
> > gimple_set_location (g, location);
> > gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> >
> > g = gimple_build_assign_with_ops (BIT_AND_EXPR,
> > make_ssa_name (boolean_type_node,
> > NULL),
> > t, gimple_assign_lhs (g));
> > gimple_set_location (g, location);
> > gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > t = gimple_assign_lhs (g);
> > -----------------------------------------------------------------------------
> >
> > I expect to add more facilities to the builder. Mainly, generation of
> > control flow altering statements which will automatically reflect on
> > the CFG.
> >
> > I do not think the helper should replace all code generation, but it
> > should serve as a shorter/simpler way of generating GIMPLE IL in the
> > common cases.
> >
> > Feedback welcome. I would like to consider adding this facility when
> > stage 1 opens.
> >
> > In the meantime, I've committed the patch to the cxx-conversion
> > branch.
> >
> >
> > Thanks. Diego.
> >
> > diff --git a/gcc/asan.c b/gcc/asan.c
> > index af9c01e..571882a 100644
> > --- a/gcc/asan.c
> > +++ b/gcc/asan.c
> > @@ -1379,57 +1379,15 @@ build_check_stmt (location_t location, tree base,
> > gimple_stmt_iterator *iter,
> > /* Slow path for 1, 2 and 4 byte accesses.
> > Test (shadow != 0)
> > & ((base_addr & 7) + (size_in_bytes - 1)) >= shadow). */
> > - g = gimple_build_assign_with_ops (NE_EXPR,
> > - make_ssa_name (boolean_type_node,
> > - NULL),
> > - shadow,
> > - build_int_cst (shadow_type, 0));
> > - gimple_set_location (g, location);
> > - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > - t = gimple_assign_lhs (g);
> > -
> > - g = gimple_build_assign_with_ops (BIT_AND_EXPR,
> > - make_ssa_name (uintptr_type,
> > - NULL),
> > - base_addr,
> > - build_int_cst (uintptr_type, 7));
> > - gimple_set_location (g, location);
> > - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > -
> > - g = gimple_build_assign_with_ops (NOP_EXPR,
> > - make_ssa_name (shadow_type,
> > - NULL),
> > - gimple_assign_lhs (g), NULL_TREE);
> > - gimple_set_location (g, location);
> > - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > -
> > + gimple_builder_ssa gb(location);
> > + t = gb.add (NE_EXPR, shadow, 0);
> > + tree t1 = gb.add (BIT_AND_EXPR, base_addr, 7);
> > + t1 = gb.add_type_cast (shadow_type, t1);
> > if (size_in_bytes > 1)
> > - {
> > - g = gimple_build_assign_with_ops (PLUS_EXPR,
> > - make_ssa_name (shadow_type,
> > - NULL),
> > - gimple_assign_lhs (g),
> > - build_int_cst (shadow_type,
> > - size_in_bytes - 1));
> > - gimple_set_location (g, location);
> > - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > - }
> > -
> > - g = gimple_build_assign_with_ops (GE_EXPR,
> > - make_ssa_name (boolean_type_node,
> > - NULL),
> > - gimple_assign_lhs (g),
> > - shadow);
> > - gimple_set_location (g, location);
> > - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > -
> > - g = gimple_build_assign_with_ops (BIT_AND_EXPR,
> > - make_ssa_name (boolean_type_node,
> > - NULL),
> > - t, gimple_assign_lhs (g));
> > - gimple_set_location (g, location);
> > - gsi_insert_after (&gsi, g, GSI_NEW_STMT);
> > - t = gimple_assign_lhs (g);
> > + t1 = gb.add (PLUS_EXPR, t1, size_in_bytes - 1);
> > + t1 = gb.add (GE_EXPR, t1, shadow);
> > + t = gb.add (BIT_AND_EXPR, t, t1);
> > + gb.insert_after (&gsi, GSI_NEW_STMT);
> > }
> > else
> > t = shadow;
> > diff --git a/gcc/gimple.c b/gcc/gimple.c
> > index 785c2f0..c4687df 100644
> > --- a/gcc/gimple.c
> > +++ b/gcc/gimple.c
> > @@ -4210,4 +4210,115 @@ gimple_asm_clobbers_memory_p (const_gimple stmt)
> >
> > return false;
> > }
> > +
> > +
> > +/* Return the expression type to use based on the CODE and type of
> > + the given operand OP. If the expression CODE is a comparison,
> > + the returned type is boolean_type_node. Otherwise, it returns
> > + the type of OP. */
> > +
> > +tree
> > +gimple_builder_base::get_expr_type (enum tree_code code, tree op)
> > +{
> > + return (TREE_CODE_CLASS (code) == tcc_comparison)
> > + ? boolean_type_node
> > + : TREE_TYPE (op);
> > +}
> > +
> > +
> > +/* Add a new assignment to this GIMPLE sequence. The assignment has
> > + the form: GIMPLE_ASSIGN <CODE, LHS, OP1, OP2>. Returns LHS. */
> > +
> > +tree
> > +gimple_builder_base::add (enum tree_code code, tree lhs, tree op1, tree
> > op2)
> > +{
> > + gimple s = gimple_build_assign_with_ops (code, lhs, op1, op2);
> > + gimple_seq_add_stmt (&seq_, s);
> > + return lhs;
> > +}
> > +
> > +
> > +/* Add a new assignment to this GIMPLE sequence. The new assignment will
> > + have the opcode CODE and operands OP1 and OP2. The type of the
> > + expression on the RHS is inferred to be the type of OP1.
> > +
> > + The LHS of the statement will be an SSA name or a GIMPLE temporary
> > + in normal form depending on the type of builder invoking this
> > + function. */
> > +
> > +tree
> > +gimple_builder_base::add (enum tree_code code, tree op1, tree op2)
> > +{
> > + tree lhs = create_lhs_for_assignment (get_expr_type (code, op1));
> > + return add (code, lhs, op1, op2);
> > +}
> > +
> > +
> > +/* Add a new assignment to this GIMPLE sequence. The new
> > + assignment will have the opcode CODE and operands OP1 and VAL.
> > + VAL is converted into a an INTEGER_CST of the same type as OP1.
> > +
> > + The LHS of the statement will be an SSA name or a GIMPLE temporary
> > + in normal form depending on the type of builder invoking this
> > + function. */
> > +
> > +tree
> > +gimple_builder_base::add (enum tree_code code, tree op1, int val)
> > +{
> > + tree type = get_expr_type (code, op1);
> > + tree op2 = build_int_cst (TREE_TYPE (op1), val);
> > + tree lhs = create_lhs_for_assignment (type);
> > + return add (code, lhs, op1, op2);
> > +}
> > +
> > +
> > +/* Add a type cast assignment to this GIMPLE sequence. This creates a
> > NOP_EXPR
> > + that converts OP to TO_TYPE. Return the LHS of the generated
> > assignment. */
> > +
> > +tree
> > +gimple_builder_base::add_type_cast (tree to_type, tree op)
> > +{
> > + tree lhs = create_lhs_for_assignment (to_type);
> > + return add (NOP_EXPR, lhs, op, NULL_TREE);
> > +}
> > +
> > +
> > +/* Insert this sequence after the statement pointed-to by iterator I.
> > + MODE is an is gs_insert_after. Scan the statements in SEQ for new
> > + operands. */
> > +
> > +void
> > +gimple_builder_base::insert_after (gimple_stmt_iterator *i,
> > + enum gsi_iterator_update mode)
> > +{
> > + /* Since we are inserting a sequence, the semantics for GSI_NEW_STMT
> > + are not quite what the caller is expecting. GSI_NEW_STMT will
> > + leave the iterator pointing to the *first* statement of this
> > + sequence. Rather, we want the iterator to point to the *last*
> > + statement in the sequence. Therefore, we use
> > + GSI_CONTINUE_LINKING when GSI_NEW_STMT is requested. */
> > + if (mode == GSI_NEW_STMT)
> > + mode = GSI_CONTINUE_LINKING;
> > + gsi_insert_seq_after (i, seq_, mode);
> > +}
> > +
> > +
> > +/* Create a GIMPLE temporary type TYPE to be used as the LHS of an
> > + assignment. */
> > +
> > +tree
> > +gimple_builder_normal::create_lhs_for_assignment (tree type)
> > +{
> > + return create_tmp_var (type, NULL);
> > +}
> > +
> > +
> > +/* Create an SSA name of type TYPE to be used as the LHS of an assignment.
> > */
> > +
> > +tree
> > +gimple_builder_ssa::create_lhs_for_assignment (tree type)
> > +{
> > + return make_ssa_name (type, NULL);
> > +}
> > +
> > #include "gt-gimple.h"
> > diff --git a/gcc/gimple.h b/gcc/gimple.h
> > index 204c3c9..7b5e741 100644
> > --- a/gcc/gimple.h
> > +++ b/gcc/gimple.h
> > @@ -5393,4 +5393,57 @@ extern tree maybe_fold_or_comparisons (enum
> > tree_code, tree, tree,
> > enum tree_code, tree, tree);
> >
> > bool gimple_val_nonnegative_real_p (tree);
> > +
> > +
> > +/* GIMPLE builder class. This type provides a simplified interface
> > + for generating new GIMPLE statements. */
> > +
> > +class gimple_builder_base
> > +{
> > +public:
> > + tree add (enum tree_code, tree, tree);
> > + tree add (enum tree_code, tree, int);
> > + tree add (enum tree_code, tree, tree, tree);
> > + tree add_type_cast (tree, tree);
> > + void insert_after (gimple_stmt_iterator *, enum gsi_iterator_update);
> > +
> > +protected:
> > + gimple_builder_base() : seq_(NULL), loc_(UNKNOWN_LOCATION) {}
> > + gimple_builder_base(location_t l) : seq_(NULL), loc_(l) {}
> > + tree get_expr_type (enum tree_code code, tree op);
> > + virtual tree create_lhs_for_assignment (tree) = 0;
> > +
> > +private:
> > + gimple_seq seq_;
> > + location_t loc_;
> > +};
> > +
> > +
> > +/* GIMPLE builder class for statements in normal form. Statements
> > generated
> > + by instances of this class will produce non-SSA temporaries. */
> > +
> > +class gimple_builder_normal : public gimple_builder_base
> > +{
> > +public:
> > + gimple_builder_normal() : gimple_builder_base() {}
> > + gimple_builder_normal(location_t l) : gimple_builder_base(l) {}
> > +
> > +protected:
> > + virtual tree create_lhs_for_assignment (tree);
> > +};
> > +
> > +
> > +/* GIMPLE builder class for statements in normal form. Statements
> > generated
> > + by instances of this class will produce SSA names. */
> > +
> > +class gimple_builder_ssa : public gimple_builder_base
> > +{
> > +public:
> > + gimple_builder_ssa() : gimple_builder_base() {}
> > + gimple_builder_ssa(location_t l) : gimple_builder_base(l) {}
> > +
> > +protected:
> > + virtual tree create_lhs_for_assignment (tree);
> > +};
> > +
> > #endif /* GCC_GIMPLE_H */
> >
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
