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 <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!
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.
