* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 21:55 [cxx-conversion] RFC - Helper types for building GIMPLE Diego Novillo
@ 2013-03-13 22:17 ` Steven Bosscher
2013-03-13 22:37 ` Diego Novillo
2013-03-13 22:19 ` Xinliang David Li
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Steven Bosscher @ 2013-03-13 22:17 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, Lawrence Crowl, David Li, Richard Biener
On Wed, Mar 13, 2013 at 10:55 PM, Diego Novillo wrote:
> This patch adds an initial implementation for a new helper type for
> generating GIMPLE statements.
Great. I was just asking you about this on IRC - you not there - but
here's a patch. Great!
> 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);
> -----------------------------------------------------------------------------
Much better than what GCC has now, although I had hoped it'd be
possible to build on the result of a previous expression without tree
variables.
> I expect to add more facilities to the builder. Mainly, generation of
> control flow altering statements which will automatically reflect on
> the CFG.
What does your plan for this look like? Personally I'd prefer to keep
CFG modifications explicit, not hidden behind this builder. CFG
changes must be done with great care, to avoid invalidating associated
data (profile, dominator tree, etc.). I'm worried that if you hide
this behind a builder, it'll be more difficult to figure out where/how
the CFG is modified (a simple grep for CFG-modifying functions won't
do anymore).
> 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.
I'd like to convert the profiling code, if you'd accept patches for
that on the cxx-branch.
> Feedback welcome. I would like to consider adding this facility when
> stage 1 opens.
Let's play with it a bit first on the CXX branch, and update
documentation like doc/gimple.texi :-)
Ciao!
Steven
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 22:17 ` Steven Bosscher
@ 2013-03-13 22:37 ` Diego Novillo
0 siblings, 0 replies; 19+ messages in thread
From: Diego Novillo @ 2013-03-13 22:37 UTC (permalink / raw)
To: Steven Bosscher; +Cc: gcc-patches, Lawrence Crowl, David Li, Richard Biener
On 2013-03-13 18:16 , Steven Bosscher wrote:
> Much better than what GCC has now, although I had hoped it'd be
> possible to build on the result of a previous expression without tree
> variables.
It may be possible, but we need something to tie the expressions
together and the temporaries are the perfect glue. I tried coming up
with some other referencing scheme, but this seemed the most natural.
If you can think of anything simpler, let's do it.
> What does your plan for this look like? Personally I'd prefer to keep
> CFG modifications explicit, not hidden behind this builder. CFG
> changes must be done with great care, to avoid invalidating associated
> data (profile, dominator tree, etc.). I'm worried that if you hide
> this behind a builder, it'll be more difficult to figure out where/how
> the CFG is modified (a simple grep for CFG-modifying functions won't
> do anymore).
Mostly what you see on the wiki. If I add a GIMPLE_COND, I want the
builder to add the basic diamond for it
(http://gcc.gnu.org/wiki/cxx-conversion/gimple-generation#Generating_statements_that_affect_control_flow)
> I'd like to convert the profiling code, if you'd accept patches for
> that on the cxx-branch.
Absolutely!
> Let's play with it a bit first on the CXX branch, and update
> documentation like doc/gimple.texi :-)
Sounds good. Though once stage 1 opens, I would prefer to move further
implementation to trunk.
Diego.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 21:55 [cxx-conversion] RFC - Helper types for building GIMPLE Diego Novillo
2013-03-13 22:17 ` Steven Bosscher
@ 2013-03-13 22:19 ` Xinliang David Li
2013-03-13 22:38 ` Diego Novillo
2013-03-14 9:00 ` Richard Biener
2013-03-13 22:27 ` Lawrence Crowl
` (3 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Xinliang David Li @ 2013-03-13 22:19 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, Lawrence Crowl, Richard Biener
Nice -- GCC LOC will be significantly reduced with these interfaces.
Using 'add' as interface name can be confusing -- new, or new_stmt, or
new_assignment/new_call etc might be better -- but we can delay the
bike shedding later.
David
On Wed, Mar 13, 2013 at 2:55 PM, Diego Novillo <dnovillo@google.com> 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:
>
>
> -----------------------------------------------------------------------------
> 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 */
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 22:19 ` Xinliang David Li
@ 2013-03-13 22:38 ` Diego Novillo
2013-03-14 9:00 ` Richard Biener
1 sibling, 0 replies; 19+ messages in thread
From: Diego Novillo @ 2013-03-13 22:38 UTC (permalink / raw)
To: Xinliang David Li; +Cc: gcc-patches, Lawrence Crowl, Richard Biener
On 2013-03-13 18:19 , Xinliang David Li wrote:
> Using 'add' as interface name can be confusing -- new, or new_stmt, or
> new_assignment/new_call etc might be better -- but we can delay the
> bike shedding later.
>
Yeah, I remember discussing this offline and I completely forgot to make
the change. I'll fix that.
Thanks. Diego.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 22:19 ` Xinliang David Li
2013-03-13 22:38 ` Diego Novillo
@ 2013-03-14 9:00 ` Richard Biener
1 sibling, 0 replies; 19+ messages in thread
From: Richard Biener @ 2013-03-14 9:00 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Diego Novillo, gcc-patches, Lawrence Crowl
On Wed, 13 Mar 2013, Xinliang David Li wrote:
> Nice -- GCC LOC will be significantly reduced with these interfaces.
>
> Using 'add' as interface name can be confusing -- new, or new_stmt, or
> new_assignment/new_call etc might be better -- but we can delay the
> bike shedding later.
Note that we already have a perfectly working "short" interface for this.
t = force_gimple_operand_gsi (... fold_build (....))
where you can in one place construct a recursive tree.
Richard.
> David.
>
> On Wed, Mar 13, 2013 at 2:55 PM, Diego Novillo <dnovillo@google.com> 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:
> >
> >
> > -----------------------------------------------------------------------------
> > 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 21:55 [cxx-conversion] RFC - Helper types for building GIMPLE Diego Novillo
2013-03-13 22:17 ` Steven Bosscher
2013-03-13 22:19 ` Xinliang David Li
@ 2013-03-13 22:27 ` Lawrence Crowl
2013-03-14 8:58 ` Richard Biener
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Lawrence Crowl @ 2013-03-13 22:27 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, David Li, Richard Biener
LGTM
On 3/13/13, Diego Novillo <dnovillo@google.com> 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:
>
>
> -----------------------------------------------------------------------------
> 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 */
>
--
Lawrence Crowl
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 21:55 [cxx-conversion] RFC - Helper types for building GIMPLE Diego Novillo
` (2 preceding siblings ...)
2013-03-13 22:27 ` Lawrence Crowl
@ 2013-03-14 8:58 ` Richard Biener
2013-03-14 9:03 ` Richard Biener
2013-04-17 9:27 ` Diego Novillo
2013-03-14 11:26 ` Michael Matz
2013-03-14 14:55 ` Marc Glisse
5 siblings, 2 replies; 19+ messages in thread
From: Richard Biener @ 2013-03-14 8:58 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, Lawrence Crowl, David Li
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 8:58 ` Richard Biener
@ 2013-03-14 9:03 ` Richard Biener
2013-03-14 11:17 ` Michael Matz
2013-04-17 9:27 ` Diego Novillo
1 sibling, 1 reply; 19+ messages in thread
From: Richard Biener @ 2013-03-14 9:03 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, Lawrence Crowl, David Li
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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 9:03 ` Richard Biener
@ 2013-03-14 11:17 ` Michael Matz
0 siblings, 0 replies; 19+ messages in thread
From: Michael Matz @ 2013-03-14 11:17 UTC (permalink / raw)
To: Richard Biener; +Cc: Diego Novillo, gcc-patches, Lawrence Crowl, David Li
Hi,
On Thu, 14 Mar 2013, Richard Biener wrote:
> That said - can we please pass in the type of the operation (and thus
> the newly created result temporary) _explicitely_ please? Thus
I'm mostly with you (not adding gimple_builder_ssa, but improving the
existing interface), except for this one. Most of the unary and binary
arithmetic and logical operations will have the the type of the first
argument as result type. Spelling that out adds useless noise, so it
shuoldn't be required. We'd need some good way to specify a type for the
rest that makes sure that you can't use the easy way with those tree
codes.
Ciao,
Michael.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 8:58 ` Richard Biener
2013-03-14 9:03 ` Richard Biener
@ 2013-04-17 9:27 ` Diego Novillo
2013-04-19 14:42 ` Richard Biener
1 sibling, 1 reply; 19+ messages in thread
From: Diego Novillo @ 2013-04-17 9:27 UTC (permalink / raw)
To: Richard Biener, gcc-patches, Lawrence Crowl, David Li, Michael Matz
Thanks for the feedback, folks.
I've removed the builder type and added some overloads to simplify the
creation of gimple assignments. I have only added exactly the functions
I needed to simplify a bit of gcc/asan.c. I plan to continue adding and
tweaking as I change client code.
Some things to note:
- The builder type gave us some more abstraction that would be nice to
put in gimple itself. However, gimple is now a union and gimple_seq is
just a typedef for gimple. So, adding behaviour to them will need to
wait until we convert gimple into a proper class.
- This variant does not yield as much code savings as the original one,
but it can be improved once gimple is a proper class.
- I will continue working in trunk. This is not something that can be
easily done in a branch since I will be touching a whole bunch of client
code and I expect to make incremental changes for the next little while.
Tested on x86_64.
2013-04-16 Diego Novillo <dnovillo@google.com>
* gimple.c (create_gimple_tmp): New.
(get_expr_type): New.
(build_assign): New.
(build_type_cast): New.
* gimple.h (enum ssa_mode): Define.
(gimple_seq_set_location): New.
* asan.c (build_check_stmt): Change some gimple_build_* calls
to use build_assign and build_type_cast.
commit a9c165448358a920e5756881e016865a812a5d81
Author: Diego Novillo <dnovillo@google.com>
Date: Tue Apr 16 14:29:09 2013 -0400
Simplified GIMPLE IL builder functions.
diff --git a/gcc/gimple.c b/gcc/gimple.c
index 8bd80c8..64f7b1a 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -4207,4 +4207,105 @@ gimple_asm_clobbers_memory_p (const_gimple stmt)
return false;
}
+
+
+/* Create and return an unnamed temporary. MODE indicates whether
+ this should be an SSA or NORMAL temporary. TYPE is the type to use
+ for the new temporary. */
+
+tree
+create_gimple_tmp (tree type, enum ssa_mode mode)
+{
+ return (mode == M_SSA)
+ ? make_ssa_name (type, NULL)
+ : create_tmp_var (type, NULL);
+}
+
+
+/* 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. */
+
+static tree
+get_expr_type (enum tree_code code, tree op)
+{
+ return (TREE_CODE_CLASS (code) == tcc_comparison)
+ ? boolean_type_node
+ : TREE_TYPE (op);
+}
+
+
+/* Build a new gimple assignment. The LHS of the assignment is a new
+ temporary whose type matches the given expression. MODE indicates
+ whether the LHS should be an SSA or a normal temporary. CODE is
+ the expression code for the RHS. OP1 is the first operand and VAL
+ is an integer value to be used as the second operand. */
+
+gimple
+build_assign (enum tree_code code, tree op1, int val, enum ssa_mode mode)
+{
+ tree op2 = build_int_cst (TREE_TYPE (op1), val);
+ tree lhs = create_gimple_tmp (get_expr_type (code, op1), mode);
+ return gimple_build_assign_with_ops (code, lhs, op1, op2);
+}
+
+gimple
+build_assign (enum tree_code code, gimple g, int val, enum ssa_mode mode)
+{
+ return build_assign (code, gimple_assign_lhs (g), val, mode);
+}
+
+
+/* Build and return a new GIMPLE assignment. 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. */
+
+gimple
+build_assign (enum tree_code code, tree op1, tree op2, enum ssa_mode mode)
+{
+ tree lhs = create_gimple_tmp (get_expr_type (code, op1), mode);
+ return gimple_build_assign_with_ops (code, lhs, op1, op2);
+}
+
+gimple
+build_assign (enum tree_code code, gimple op1, tree op2, enum ssa_mode
mode)
+{
+ return build_assign (code, gimple_assign_lhs (op1), op2, mode);
+}
+
+gimple
+build_assign (enum tree_code code, tree op1, gimple op2, enum ssa_mode
mode)
+{
+ return build_assign (code, op1, gimple_assign_lhs (op2), mode);
+}
+
+gimple
+build_assign (enum tree_code code, gimple op1, gimple op2, enum
ssa_mode mode)
+{
+ return build_assign (code, gimple_assign_lhs (op1), gimple_assign_lhs
(op2),
+ mode);
+}
+
+
+/* Create and return a type cast assignment. This creates a NOP_EXPR
+ that converts OP to TO_TYPE. */
+
+gimple
+build_type_cast (tree to_type, tree op, enum ssa_mode mode)
+{
+ tree lhs = create_gimple_tmp (to_type, mode);
+ return gimple_build_assign_with_ops (NOP_EXPR, lhs, op, NULL_TREE);
+}
+
+gimple
+build_type_cast (tree to_type, gimple op, enum ssa_mode mode)
+{
+ return build_type_cast (to_type, gimple_assign_lhs (op), mode);
+}
+
#include "gt-gimple.h"
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 475d2ea..3a65e3c 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -33,6 +33,15 @@ along with GCC; see the file COPYING3. If not see
typedef gimple gimple_seq_node;
+/* Types of supported temporaries. GIMPLE temporaries may be symbols
+ in normal form (i.e., regular decls) or SSA names. This enum is
+ used by create_gimple_tmp to tell it what kind of temporary the
+ caller wants. */
+enum ssa_mode {
+ M_SSA = 0,
+ M_NORMAL
+};
+
/* For each block, the PHI nodes that need to be rewritten are stored into
these vectors. */
typedef vec<gimple> gimple_vec;
@@ -720,6 +729,17 @@ union GTY ((desc ("gimple_statement_structure (&%h)"),
/* In gimple.c. */
+/* Helper functions to build GIMPLE statements. */
+tree create_gimple_tmp (tree, enum ssa_mode = M_SSA);
+gimple build_assign (enum tree_code, tree, int, enum ssa_mode = M_SSA);
+gimple build_assign (enum tree_code, gimple, int, enum ssa_mode = M_SSA);
+gimple build_assign (enum tree_code, tree, tree, enum ssa_mode = M_SSA);
+gimple build_assign (enum tree_code, gimple, tree, enum ssa_mode = M_SSA);
+gimple build_assign (enum tree_code, tree, gimple, enum ssa_mode = M_SSA);
+gimple build_assign (enum tree_code, gimple, gimple, enum ssa_mode =
M_SSA);
+gimple build_type_cast (tree, tree, enum ssa_mode = M_SSA);
+gimple build_type_cast (tree, gimple, enum ssa_mode = M_SSA);
+
/* Offset in bytes to the location of the operand vector.
Zero if there is no operand vector for this tuple structure. */
extern size_t const gimple_ops_offset_[];
@@ -1096,7 +1116,6 @@ gimple_seq_empty_p (gimple_seq s)
return s == NULL;
}
-
void gimple_seq_add_stmt (gimple_seq *, gimple);
/* Link gimple statement GS to the end of the sequence *SEQ_P. If
@@ -5326,4 +5345,15 @@ extern tree maybe_fold_or_comparisons (enum
tree_code, tree, tree,
enum tree_code, tree, tree);
bool gimple_val_nonnegative_real_p (tree);
+
+
+/* Set the location of all statements in SEQ to LOC. */
+
+static inline void
+gimple_seq_set_location (gimple_seq seq, location_t loc)
+{
+ for (gimple_stmt_iterator i = gsi_start (seq); !gsi_end_p (i);
gsi_next (&i))
+ gimple_set_location (gsi_stmt (i), loc);
+}
+
#endif /* GCC_GIMPLE_H */
diff --git a/gcc/asan.c b/gcc/asan.c
index 36eccf9..b8acaf7 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1380,57 +1380,23 @@ 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_seq seq = NULL;
+ gimple shadow_test = build_assign (NE_EXPR, shadow, 0);
+ gimple_seq_add_stmt (&seq, shadow_test);
+ gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, base_addr,
7));
+ gimple_seq_add_stmt (&seq, build_type_cast (shadow_type,
+ gimple_seq_last (seq)));
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);
+ gimple_seq_add_stmt (&seq,
+ build_assign (PLUS_EXPR, gimple_seq_last
(seq),
+ size_in_bytes - 1));
+ gimple_seq_add_stmt (&seq, build_assign (GE_EXPR, gimple_seq_last
(seq),
+ shadow));
+ gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test,
+ gimple_seq_last (seq)));
+ t = gimple_assign_lhs (gimple_seq_last (seq));
+ gimple_seq_set_location (seq, location);
+ gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING);
}
else
t = shadow;
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-04-17 9:27 ` Diego Novillo
@ 2013-04-19 14:42 ` Richard Biener
2013-04-24 18:20 ` Diego Novillo
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2013-04-19 14:42 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, Lawrence Crowl, David Li, Michael Matz
On Tue, 16 Apr 2013, Diego Novillo wrote:
> Thanks for the feedback, folks.
>
> I've removed the builder type and added some overloads to simplify the
> creation of gimple assignments. I have only added exactly the functions I
> needed to simplify a bit of gcc/asan.c. I plan to continue adding and
> tweaking as I change client code.
>
> Some things to note:
>
> - The builder type gave us some more abstraction that would be nice to put in
> gimple itself. However, gimple is now a union and gimple_seq is just a
> typedef for gimple. So, adding behaviour to them will need to wait until we
> convert gimple into a proper class.
>
> - This variant does not yield as much code savings as the original one, but it
> can be improved once gimple is a proper class.
>
> - I will continue working in trunk. This is not something that can be easily
> done in a branch since I will be touching a whole bunch of client code and I
> expect to make incremental changes for the next little while.
>
>
> Tested on x86_64.
Comments inoine below.
> 2013-04-16 Diego Novillo <dnovillo@google.com>
>
> * gimple.c (create_gimple_tmp): New.
> (get_expr_type): New.
> (build_assign): New.
> (build_type_cast): New.
> * gimple.h (enum ssa_mode): Define.
> (gimple_seq_set_location): New.
> * asan.c (build_check_stmt): Change some gimple_build_* calls
> to use build_assign and build_type_cast.
>
> commit a9c165448358a920e5756881e016865a812a5d81
> Author: Diego Novillo <dnovillo@google.com>
> Date: Tue Apr 16 14:29:09 2013 -0400
>
> Simplified GIMPLE IL builder functions.
>
> diff --git a/gcc/gimple.c b/gcc/gimple.c
> index 8bd80c8..64f7b1a 100644
> --- a/gcc/gimple.c
> +++ b/gcc/gimple.c
> @@ -4207,4 +4207,105 @@ gimple_asm_clobbers_memory_p (const_gimple stmt)
>
> return false;
> }
> +
> +
> +/* Create and return an unnamed temporary. MODE indicates whether
> + this should be an SSA or NORMAL temporary. TYPE is the type to use
> + for the new temporary. */
> +
> +tree
> +create_gimple_tmp (tree type, enum ssa_mode mode)
> +{
> + return (mode == M_SSA)
> + ? make_ssa_name (type, NULL)
> + : create_tmp_var (type, NULL);
> +}
Eh - what exactly is this for? It doesn't simplify anything!
> +
> +/* 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. */
> +
> +static tree
> +get_expr_type (enum tree_code code, tree op)
> +{
> + return (TREE_CODE_CLASS (code) == tcc_comparison)
> + ? boolean_type_node
> + : TREE_TYPE (op);
> +}
Which returns wrong results for FIX_TRUNC_EXPR and a double op.
This function cannot be implemented correctly with the given
signature (read: the type of the expression cannot be determined
by just looking at 'code' and 'op' in all cases). Drop it.
> +
> +/* Build a new gimple assignment. The LHS of the assignment is a new
> + temporary whose type matches the given expression. MODE indicates
> + whether the LHS should be an SSA or a normal temporary. CODE is
> + the expression code for the RHS. OP1 is the first operand and VAL
> + is an integer value to be used as the second operand. */
> +
> +gimple
> +build_assign (enum tree_code code, tree op1, int val, enum ssa_mode mode)
> +{
> + tree op2 = build_int_cst (TREE_TYPE (op1), val);
> + tree lhs = create_gimple_tmp (get_expr_type (code, op1), mode);
> + return gimple_build_assign_with_ops (code, lhs, op1, op2);
> +}
This 'mode' thingie is broken. Make that beast auto-detected
(gimple_in_ssa_p && is_gimple_reg_type).
Why not start simple and continue my overloading patches
(which dropped gimple_build_assign_with_ops3) to make all the
gimple stmt builders overloads of a single
gimple_build_assing ()
? Do it incrementally though, as I expect that with each new overload
one function goes away and users are adjusted. That way you also
get testing coverage - which your patch has none.
> +gimple
> +build_assign (enum tree_code code, gimple g, int val, enum ssa_mode mode)
> +{
> + return build_assign (code, gimple_assign_lhs (g), val, mode);
> +}
> +
> +
> +/* Build and return a new GIMPLE assignment. 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. */
> +
> +gimple
> +build_assign (enum tree_code code, tree op1, tree op2, enum ssa_mode mode)
> +{
> + tree lhs = create_gimple_tmp (get_expr_type (code, op1), mode);
> + return gimple_build_assign_with_ops (code, lhs, op1, op2);
> +}
> +
> +gimple
> +build_assign (enum tree_code code, gimple op1, tree op2, enum ssa_mode mode)
> +{
> + return build_assign (code, gimple_assign_lhs (op1), op2, mode);
> +}
> +
> +gimple
> +build_assign (enum tree_code code, tree op1, gimple op2, enum ssa_mode mode)
> +{
> + return build_assign (code, op1, gimple_assign_lhs (op2), mode);
> +}
> +
> +gimple
> +build_assign (enum tree_code code, gimple op1, gimple op2, enum ssa_mode
> mode)
> +{
> + return build_assign (code, gimple_assign_lhs (op1), gimple_assign_lhs
> (op2),
> + mode);
> +}
> +
> +/* Create and return a type cast assignment. This creates a NOP_EXPR
> + that converts OP to TO_TYPE. */
> +
> +gimple
> +build_type_cast (tree to_type, tree op, enum ssa_mode mode)
> +{
> + tree lhs = create_gimple_tmp (to_type, mode);
> + return gimple_build_assign_with_ops (NOP_EXPR, lhs, op, NULL_TREE);
> +}
> +
> +gimple
> +build_type_cast (tree to_type, gimple op, enum ssa_mode mode)
> +{
> + return build_type_cast (to_type, gimple_assign_lhs (op), mode);
> +}
I'd say it should be
tree
gimple_convert (gimple_stmt_iterator *gsi, tree type, tree op)
which converts op to type, returning either 'op' (it's type is
compatible to 'type') or a new register temporary (please ICE
on !is_gimple_reg_type converts!) which initialization is
inserted at gsi (eventually needs an extra param for
the iterator update kind - unless we standardize on GSI_CONTINUE_LINKING
for all 'builders' which would make sense).
This gimple_convert should be used to replace all fold_convert
calls in the various passes (well, those that end up re-gimplifying
the result).
> #include "gt-gimple.h"
> diff --git a/gcc/gimple.h b/gcc/gimple.h
> index 475d2ea..3a65e3c 100644
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -33,6 +33,15 @@ along with GCC; see the file COPYING3. If not see
>
> typedef gimple gimple_seq_node;
>
> +/* Types of supported temporaries. GIMPLE temporaries may be symbols
> + in normal form (i.e., regular decls) or SSA names. This enum is
> + used by create_gimple_tmp to tell it what kind of temporary the
> + caller wants. */
> +enum ssa_mode {
> + M_SSA = 0,
> + M_NORMAL
> +};
> +
> /* For each block, the PHI nodes that need to be rewritten are stored into
> these vectors. */
> typedef vec<gimple> gimple_vec;
> @@ -720,6 +729,17 @@ union GTY ((desc ("gimple_statement_structure (&%h)"),
>
> /* In gimple.c. */
>
> +/* Helper functions to build GIMPLE statements. */
> +tree create_gimple_tmp (tree, enum ssa_mode = M_SSA);
> +gimple build_assign (enum tree_code, tree, int, enum ssa_mode = M_SSA);
> +gimple build_assign (enum tree_code, gimple, int, enum ssa_mode = M_SSA);
> +gimple build_assign (enum tree_code, tree, tree, enum ssa_mode = M_SSA);
> +gimple build_assign (enum tree_code, gimple, tree, enum ssa_mode = M_SSA);
> +gimple build_assign (enum tree_code, tree, gimple, enum ssa_mode = M_SSA);
> +gimple build_assign (enum tree_code, gimple, gimple, enum ssa_mode = M_SSA);
> +gimple build_type_cast (tree, tree, enum ssa_mode = M_SSA);
> +gimple build_type_cast (tree, gimple, enum ssa_mode = M_SSA);
> +
> /* Offset in bytes to the location of the operand vector.
> Zero if there is no operand vector for this tuple structure. */
> extern size_t const gimple_ops_offset_[];
> @@ -1096,7 +1116,6 @@ gimple_seq_empty_p (gimple_seq s)
> return s == NULL;
> }
>
> -
> void gimple_seq_add_stmt (gimple_seq *, gimple);
>
> /* Link gimple statement GS to the end of the sequence *SEQ_P. If
> @@ -5326,4 +5345,15 @@ extern tree maybe_fold_or_comparisons (enum tree_code,
> tree, tree,
> enum tree_code, tree, tree);
>
> bool gimple_val_nonnegative_real_p (tree);
> +
> +
> +/* Set the location of all statements in SEQ to LOC. */
> +
> +static inline void
> +gimple_seq_set_location (gimple_seq seq, location_t loc)
> +{
> + for (gimple_stmt_iterator i = gsi_start (seq); !gsi_end_p (i); gsi_next
> (&i))
> + gimple_set_location (gsi_stmt (i), loc);
> +}
Rather than this the gsi_insert_* routines accepting a sequence should
get an optional location argument? OTOH the above is orthogonal to that.
Btw, the above should assert that we don't overwrite an existing
"good" location.
Richard.
> #endif /* GCC_GIMPLE_H */
> diff --git a/gcc/asan.c b/gcc/asan.c
> index 36eccf9..b8acaf7 100644
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -1380,57 +1380,23 @@ 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_seq seq = NULL;
> + gimple shadow_test = build_assign (NE_EXPR, shadow, 0);
> + gimple_seq_add_stmt (&seq, shadow_test);
> + gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, base_addr, 7));
> + gimple_seq_add_stmt (&seq, build_type_cast (shadow_type,
> + gimple_seq_last (seq)));
> 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);
> + gimple_seq_add_stmt (&seq,
> + build_assign (PLUS_EXPR, gimple_seq_last (seq),
> + size_in_bytes - 1));
> + gimple_seq_add_stmt (&seq, build_assign (GE_EXPR, gimple_seq_last
> (seq),
> + shadow));
> + gimple_seq_add_stmt (&seq, build_assign (BIT_AND_EXPR, shadow_test,
> + gimple_seq_last (seq)));
> + t = gimple_assign_lhs (gimple_seq_last (seq));
> + gimple_seq_set_location (seq, location);
> + gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING);
> }
> else
> t = shadow;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-04-19 14:42 ` Richard Biener
@ 2013-04-24 18:20 ` Diego Novillo
0 siblings, 0 replies; 19+ messages in thread
From: Diego Novillo @ 2013-04-24 18:20 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Lawrence Crowl, David Li, Michael Matz
On 2013-04-19 07:30 , Richard Biener wrote:
> On Tue, 16 Apr 2013, Diego Novillo wrote:
>
>> Thanks for the feedback, folks.
>>
>> I've removed the builder type and added some overloads to simplify the
>> creation of gimple assignments. I have only added exactly the functions I
>> needed to simplify a bit of gcc/asan.c. I plan to continue adding and
>> tweaking as I change client code.
>>
>> Some things to note:
>>
>> - The builder type gave us some more abstraction that would be nice to put in
>> gimple itself. However, gimple is now a union and gimple_seq is just a
>> typedef for gimple. So, adding behaviour to them will need to wait until we
>> convert gimple into a proper class.
>>
>> - This variant does not yield as much code savings as the original one, but it
>> can be improved once gimple is a proper class.
>>
>> - I will continue working in trunk. This is not something that can be easily
>> done in a branch since I will be touching a whole bunch of client code and I
>> expect to make incremental changes for the next little while.
>>
>>
>> Tested on x86_64.
> Comments inoine below.
>
>
>> 2013-04-16 Diego Novillo <dnovillo@google.com>
>>
>> * gimple.c (create_gimple_tmp): New.
>> (get_expr_type): New.
>> (build_assign): New.
>> (build_type_cast): New.
>> * gimple.h (enum ssa_mode): Define.
>> (gimple_seq_set_location): New.
>> * asan.c (build_check_stmt): Change some gimple_build_* calls
>> to use build_assign and build_type_cast.
>>
>> commit a9c165448358a920e5756881e016865a812a5d81
>> Author: Diego Novillo <dnovillo@google.com>
>> Date: Tue Apr 16 14:29:09 2013 -0400
>>
>> Simplified GIMPLE IL builder functions.
>>
>> diff --git a/gcc/gimple.c b/gcc/gimple.c
>> index 8bd80c8..64f7b1a 100644
>> --- a/gcc/gimple.c
>> +++ b/gcc/gimple.c
>> @@ -4207,4 +4207,105 @@ gimple_asm_clobbers_memory_p (const_gimple stmt)
>>
>> return false;
>> }
>> +
>> +
>> +/* Create and return an unnamed temporary. MODE indicates whether
>> + this should be an SSA or NORMAL temporary. TYPE is the type to use
>> + for the new temporary. */
>> +
>> +tree
>> +create_gimple_tmp (tree type, enum ssa_mode mode)
>> +{
>> + return (mode == M_SSA)
>> + ? make_ssa_name (type, NULL)
>> + : create_tmp_var (type, NULL);
>> +}
> Eh - what exactly is this for? It doesn't simplify anything!
Helper for the other builders. Should be private to gimple.c.
>
>> +
>> +/* 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. */
>> +
>> +static tree
>> +get_expr_type (enum tree_code code, tree op)
>> +{
>> + return (TREE_CODE_CLASS (code) == tcc_comparison)
>> + ? boolean_type_node
>> + : TREE_TYPE (op);
>> +}
> Which returns wrong results for FIX_TRUNC_EXPR and a double op.
> This function cannot be implemented correctly with the given
> signature (read: the type of the expression cannot be determined
> by just looking at 'code' and 'op' in all cases). Drop it.
Hmm, yeah. I will.
> This 'mode' thingie is broken. Make that beast auto-detected
> (gimple_in_ssa_p && is_gimple_reg_type).
I don't like that. This would make it context sensitive. We should
move away from these magic globals.
>
> Why not start simple and continue my overloading patches
> (which dropped gimple_build_assign_with_ops3) to make all the
> gimple stmt builders overloads of a single
>
> gimple_build_assing ()
>
> ?
That was kind of the idea. But I started at a different place. I'll
keep adding overloads and converting more client code.
> Do it incrementally though, as I expect that with each new overload
> one function goes away and users are adjusted. That way you also
> get testing coverage - which your patch has none.
Oh, it does. All the asan tests exercise it.
> +/* Create and return a type cast assignment. This creates a NOP_EXPR
> + that converts OP to TO_TYPE. */
> +
> +gimple
> +build_type_cast (tree to_type, tree op, enum ssa_mode mode)
> +{
> + tree lhs = create_gimple_tmp (to_type, mode);
> + return gimple_build_assign_with_ops (NOP_EXPR, lhs, op, NULL_TREE);
> +}
> +
> +gimple
> +build_type_cast (tree to_type, gimple op, enum ssa_mode mode)
> +{
> + return build_type_cast (to_type, gimple_assign_lhs (op), mode);
> +}
> I'd say it should be
>
> tree
> gimple_convert (gimple_stmt_iterator *gsi, tree type, tree op)
>
> which converts op to type, returning either 'op' (it's type is
> compatible to 'type') or a new register temporary (please ICE
> on !is_gimple_reg_type converts!) which initialization is
> inserted at gsi (eventually needs an extra param for
> the iterator update kind - unless we standardize on GSI_CONTINUE_LINKING
> for all 'builders' which would make sense).
>
> This gimple_convert should be used to replace all fold_convert
> calls in the various passes (well, those that end up re-gimplifying
> the result).
Why so many side-effects? The reason I'm returning gimple is so that it
can be used as an argument in more builders. They use the LHS.
>
>> bool gimple_val_nonnegative_real_p (tree);
>> +
>> +
>> +/* Set the location of all statements in SEQ to LOC. */
>> +
>> +static inline void
>> +gimple_seq_set_location (gimple_seq seq, location_t loc)
>> +{
>> + for (gimple_stmt_iterator i = gsi_start (seq); !gsi_end_p (i); gsi_next
>> (&i))
>> + gimple_set_location (gsi_stmt (i), loc);
>> +}
> Rather than this the gsi_insert_* routines accepting a sequence should
> get an optional location argument? OTOH the above is orthogonal to that.
Will do.
Diego.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 21:55 [cxx-conversion] RFC - Helper types for building GIMPLE Diego Novillo
` (3 preceding siblings ...)
2013-03-14 8:58 ` Richard Biener
@ 2013-03-14 11:26 ` Michael Matz
2013-03-14 16:17 ` Xinliang David Li
2013-03-14 14:55 ` Marc Glisse
5 siblings, 1 reply; 19+ messages in thread
From: Michael Matz @ 2013-03-14 11:26 UTC (permalink / raw)
To: Diego Novillo; +Cc: gcc-patches, Lawrence Crowl, David Li, Richard Biener
Hi,
On Wed, 13 Mar 2013, Diego Novillo wrote:
> 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.
I'm reiterating everything I said in
http://gcc.gnu.org/ml/gcc/2012-11/msg00230.html
Actually your interface now is worse than what Lawrence proposed back in
November in that the add_* methods return a tree, instead of a gimple
statement.
I haven't seen any convincing reason why the builder class is necessary,
instead of improving the current interface. IOW I don't like it much.
Ciao,
Michael.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 11:26 ` Michael Matz
@ 2013-03-14 16:17 ` Xinliang David Li
2013-03-14 17:53 ` Richard Biener
0 siblings, 1 reply; 19+ messages in thread
From: Xinliang David Li @ 2013-03-14 16:17 UTC (permalink / raw)
To: Michael Matz; +Cc: Diego Novillo, gcc-patches, Lawrence Crowl, Richard Biener
I am with you for simple cases where straightline code is generated.
Helper class will be very useful when control flow manipulation is
involved. People can simply just write 'straight line like code'
using gimple_label, goto_label etc without worrying about how split
blocks and create new cfg edges. The 'end/finalize' method of the
builder helper will do the magic.
David
On Thu, Mar 14, 2013 at 4:25 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 13 Mar 2013, Diego Novillo wrote:
>
>> 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.
>
> I'm reiterating everything I said in
> http://gcc.gnu.org/ml/gcc/2012-11/msg00230.html
> Actually your interface now is worse than what Lawrence proposed back in
> November in that the add_* methods return a tree, instead of a gimple
> statement.
>
> I haven't seen any convincing reason why the builder class is necessary,
> instead of improving the current interface. IOW I don't like it much.
>
>
> Ciao,
> Michael.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 16:17 ` Xinliang David Li
@ 2013-03-14 17:53 ` Richard Biener
2013-03-14 18:48 ` Xinliang David Li
0 siblings, 1 reply; 19+ messages in thread
From: Richard Biener @ 2013-03-14 17:53 UTC (permalink / raw)
To: Xinliang David Li, Michael Matz
Cc: Diego Novillo, gcc-patches, Lawrence Crowl
Xinliang David Li <davidxl@google.com> wrote:
>I am with you for simple cases where straightline code is generated.
>
>Helper class will be very useful when control flow manipulation is
>involved. People can simply just write 'straight line like code'
>using gimple_label, goto_label etc without worrying about how split
>blocks and create new cfg edges. The 'end/finalize' method of the
>builder helper will do the magic.
That won't work with the magic in the builder to create ssa names for the results. Then people need to think about cfg merges and phi nodes. So much for labels and gotos...
I'd rather have people know about the context they are working in. Otherwise they cannot exploit its property either.
Richard.
>David
>
>On Thu, Mar 14, 2013 at 4:25 AM, Michael Matz <matz@suse.de> wrote:
>> Hi,
>>
>> On Wed, 13 Mar 2013, Diego Novillo wrote:
>>
>>> 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.
>>
>> I'm reiterating everything I said in
>> http://gcc.gnu.org/ml/gcc/2012-11/msg00230.html
>> Actually your interface now is worse than what Lawrence proposed back
>in
>> November in that the add_* methods return a tree, instead of a gimple
>> statement.
>>
>> I haven't seen any convincing reason why the builder class is
>necessary,
>> instead of improving the current interface. IOW I don't like it
>much.
>>
>>
>> Ciao,
>> Michael.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 17:53 ` Richard Biener
@ 2013-03-14 18:48 ` Xinliang David Li
0 siblings, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2013-03-14 18:48 UTC (permalink / raw)
To: Richard Biener; +Cc: Michael Matz, Diego Novillo, gcc-patches, Lawrence Crowl
On Thu, Mar 14, 2013 at 10:54 AM, Richard Biener <rguenther@suse.de> wrote:
> Xinliang David Li <davidxl@google.com> wrote:
>
>>I am with you for simple cases where straightline code is generated.
>>
>>Helper class will be very useful when control flow manipulation is
>>involved. People can simply just write 'straight line like code'
>>using gimple_label, goto_label etc without worrying about how split
>>blocks and create new cfg edges. The 'end/finalize' method of the
>>builder helper will do the magic.
>
> That won't work with the magic in the builder to create ssa names for the results. Then people need to think about cfg merges and phi nodes. So much for labels and gotos...
>
> I'd rather have people know about the context they are working in. Otherwise they cannot exploit its property either.
I am not sure what you mean. The builder helper class should contain
all the context information which should be accessible if user wants
to take a peek. The low level information is usually needed for
analysis, but not needs to be exposed for transformation.
David
>
> Richard.
>>David
>>
>>On Thu, Mar 14, 2013 at 4:25 AM, Michael Matz <matz@suse.de> wrote:
>>> Hi,
>>>
>>> On Wed, 13 Mar 2013, Diego Novillo wrote:
>>>
>>>> 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.
>>>
>>> I'm reiterating everything I said in
>>> http://gcc.gnu.org/ml/gcc/2012-11/msg00230.html
>>> Actually your interface now is worse than what Lawrence proposed back
>>in
>>> November in that the add_* methods return a tree, instead of a gimple
>>> statement.
>>>
>>> I haven't seen any convincing reason why the builder class is
>>necessary,
>>> instead of improving the current interface. IOW I don't like it
>>much.
>>>
>>>
>>> Ciao,
>>> Michael.
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-13 21:55 [cxx-conversion] RFC - Helper types for building GIMPLE Diego Novillo
` (4 preceding siblings ...)
2013-03-14 11:26 ` Michael Matz
@ 2013-03-14 14:55 ` Marc Glisse
2013-03-14 16:19 ` Xinliang David Li
5 siblings, 1 reply; 19+ messages in thread
From: Marc Glisse @ 2013-03-14 14:55 UTC (permalink / raw)
To: gcc-patches
On Wed, 13 Mar 2013, Diego Novillo wrote:
> This patch adds an initial implementation for a new helper type for
> generating GIMPLE statements.
I hope you'll forgive the naive newbie question: why is the gimplifier
used so little outside of the gimplification pass? For instance, after a
call to fold in a gimple pass, we test valid_gimple_rhs_p and give up if
it returns false, instead of trying to gimplify whatever fold returned.
The connection with your patch is that generic trees are easier to build
than gimple statements, so for a long expression one could imagine
building a tree and having a single gimplification call at the end (the
time wasted should be quite limited).
(Note that I encourage the efforts to simplify the gimple interface, I am
just taking the occasion to ask a vaguely related question)
--
Marc Glisse
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [cxx-conversion] RFC - Helper types for building GIMPLE
2013-03-14 14:55 ` Marc Glisse
@ 2013-03-14 16:19 ` Xinliang David Li
0 siblings, 0 replies; 19+ messages in thread
From: Xinliang David Li @ 2013-03-14 16:19 UTC (permalink / raw)
To: gcc-patches
On Thu, Mar 14, 2013 at 7:55 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Wed, 13 Mar 2013, Diego Novillo wrote:
>
>> This patch adds an initial implementation for a new helper type for
>> generating GIMPLE statements.
>
>
> I hope you'll forgive the naive newbie question: why is the gimplifier used
> so little outside of the gimplification pass? For instance, after a call to
> fold in a gimple pass, we test valid_gimple_rhs_p and give up if it returns
> false, instead of trying to gimplify whatever fold returned. The connection
> with your patch is that generic trees are easier to build than gimple
> statements, so for a long expression one could imagine building a tree and
> having a single gimplification call at the end (the time wasted should be
> quite limited).
It also creates more garbage and increase ggc overhead?
David
>
> (Note that I encourage the efforts to simplify the gimple interface, I am
> just taking the occasion to ask a vaguely related question)
>
> --
> Marc Glisse
^ permalink raw reply [flat|nested] 19+ messages in thread