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 <dnovi...@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 <dnovi...@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;
>
>
