This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [cxx-conversion] RFC - Helper types for building GIMPLE


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]