| Message ID | 20130313215516.GA29289@google.com |
|---|---|
| State | New |
| Headers | show |
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
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 */
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 */ >
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.
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.
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 */ > >
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 */ > >
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 */ > > > > > >
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.
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.
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)
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.
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
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.
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. > >
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 */
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.