| Message ID | 516DBFBF.50608@google.com |
|---|---|
| State | New |
| Headers | show |
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; > >
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.
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);
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. + gsi_insert_seq_after (&gsi, seq, GSI_CONTINUE_LINKING); } else t = shadow;