Re: [cxx-conversion] RFC - Helper types for building GIMPLE

[Richard Biener <rguenther at suse dot de>]

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 <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend