This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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'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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]