From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18594 invoked by alias); 14 Mar 2013 08:58:06 -0000 Received: (qmail 18586 invoked by uid 22791); 14 Mar 2013 08:58:06 -0000 X-SWARE-Spam-Status: No, hits=-7.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,TW_CX,TW_TM X-Spam-Check-By: sourceware.org Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 14 Mar 2013 08:57:56 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 19847A51F7; Thu, 14 Mar 2013 09:57:55 +0100 (CET) Date: Thu, 14 Mar 2013 08:58:00 -0000 From: Richard Biener To: Diego Novillo Cc: gcc-patches@gcc.gnu.org, Lawrence Crowl , David Li Subject: Re: [cxx-conversion] RFC - Helper types for building GIMPLE In-Reply-To: <20130313215516.GA29289@google.com> Message-ID: References: <20130313215516.GA29289@google.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-03/txt/msg00491.txt.bz2 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 . 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 */ > > -- Richard Biener SUSE / SUSE Labs SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 GF: Jeff Hawn, Jennifer Guild, Felix Imend