From patchwork Thu Aug 5 13:17:51 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 60971 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id A7D5EB6F06 for ; Thu, 5 Aug 2010 23:18:07 +1000 (EST) Received: (qmail 22250 invoked by alias); 5 Aug 2010 13:18:05 -0000 Received: (qmail 22208 invoked by uid 22791); 5 Aug 2010 13:18:02 -0000 X-SWARE-Spam-Status: No, hits=-3.7 required=5.0 tests=AWL,BAYES_00 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, 05 Aug 2010 13:17:55 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) by mx2.suse.de (Postfix) with ESMTP id 67A7087D82; Thu, 5 Aug 2010 15:17:52 +0200 (CEST) Date: Thu, 5 Aug 2010 15:17:51 +0200 From: Martin Jambor To: Richard Guenther Cc: GCC Patches , Richard Guenther Subject: Re: [RFC PATCH] Remove call to build_ref_for_offset in IPA-SRA Message-ID: <20100805131751.GC1800@virgil.arch.suse.de> Mail-Followup-To: Richard Guenther , GCC Patches , Richard Guenther References: <20100802202036.GC18246@virgil.arch.suse.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi, On Tue, Aug 03, 2010 at 12:03:03PM +0200, Richard Guenther wrote: > On Mon, Aug 2, 2010 at 10:20 PM, Martin Jambor wrote: > > Hi, > > > > the following is another patch in the series that eventually should > > re-implement build_ref_for_offset.  It basically re-implements it in > > the IPA-SRA user of the function, I might eventually decide to use the > > new implementation of the function instead but I fairly like it > > separate because this (current) user of the function 1) always gets > > offsets aligned to bytes and 2) it can be fed pointers, unlike all > > others. > > > > Anyway, the problem with this patch is of course the hunk in expr.c. > > However, without it I get ICEs when compiling testcase > > g++.dg/torture/pr36445.C on x86_64-linux even though I believe I > > produce correct gimple.  As far as I understand the code, if the V_C_E > > path is not taken and we try to build and expand a BIT_FIELD_REF, I > > get something that is not BLKmode because its argument is not and this > > is not expected by emit_block_move which attempts to use it.  I am not > > sure what exactly and exactly under what circumstances needs to behave > > differently.  I intend to go back to this problem but I am new to the > > code, and thus I would appreciate any help with that.  Otherwise the > > patch bootstraps and tests fine. > > > > Thanks, > > > > Martin > > > > > > 2010-07-28 Martin Jambor > > > > * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of > > calling build_ref_for_offset. > > * expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also > > when sizes don't match but expected mode is BLKmode. > > > > * testsuite/g++.dg/torture/pr34850.C: Remove expected warning. > > > > Index: mine/gcc/ipa-prop.c > > =================================================================== > > --- mine.orig/gcc/ipa-prop.c > > +++ mine/gcc/ipa-prop.c > > @@ -2149,40 +2149,63 @@ ipa_modify_call_arguments (struct cgraph > > } > > else if (!adj->remove_param) > > { > > - tree expr, orig_expr; > > - bool allow_ptr, repl_found; > > + tree expr, base, off; > > + location_t loc; > > > > - orig_expr = expr = gimple_call_arg (stmt, adj->base_index); > > - if (TREE_CODE (expr) == ADDR_EXPR) > > - { > > - allow_ptr = false; > > - expr = TREE_OPERAND (expr, 0); > > - } > > - else > > - allow_ptr = true; > > + gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0); > > + base = gimple_call_arg (stmt, adj->base_index); > > + loc = EXPR_LOCATION (base); > > If I read the following code correctly we build an access to > &base + offset here? The old allow_ptr code and the > replacement looks odd. Can't we just store a 'deref_p' flag > in the ipa-parm adjustment structure? > > Please add a comment what we are doing here. > > What are we actually replacing? A ptr argument that is > dereferenced by a scalar arg? An aggregate by-value arg > with separate by-value args? Note that this is the part where we transform calls in callees of the function we are actually transforming. I'm adding the following comment: /* We create a new parameter out of the value of the old one, we can do the following kind of transformations: - A scalar passed by reference is converted to a scalar passed by value. (adj->by_ref is false and the type of the original actual argument is a pointer to a scalar). - A part of an aggregate is passed instead of the whole aggregate. The part can be passed either by value or by reference, this is determined by value of adj->by_ref. Moreover, the code below handles both situations when the original aggregate is passed by value (its type is not a pointer) and when it is passed by reference (it is a pointer to an aggregate). When the new argument is passed by reference (adj->by_ref is true) it must be a part of an aggregate and therefore we form it by simply taking the address of a reference inside the original aggregate. */ Turning scalars for which we have a decl in this function and which are passed by reference into arguments by value is currently really done by folding the resulting MEM_REF. Perhaps I should special-case it here. But sometimes we're just dereferencing a pointer in the caller and don't have a decl. > > > - repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr), > > - adj->offset, adj->type, > > - allow_ptr); > > - if (repl_found) > > + if ((TREE_CODE (base) == ADDR_EXPR > > + && DECL_P (TREE_OPERAND (base, 0))) > > + || (TREE_CODE (base) != ADDR_EXPR > > + && POINTER_TYPE_P (TREE_TYPE (base)))) > > + off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT); > > + else > > { > > - if (adj->by_ref) > > - expr = build_fold_addr_expr (expr); > > + HOST_WIDE_INT base_offset; > > + tree prev_base; > > + > > + if (TREE_CODE (base) == ADDR_EXPR) > > + base = TREE_OPERAND (base, 0); > > + prev_base = base; > > + base = get_addr_base_and_unit_offset (base, &base_offset); > > + if (!base) > > Ugh - when does that happen? (it shouldn't) Any time we bump into an array_ref with a non-constant index (such as in ptr->array[i].structure). I believe we actually discussed this on IRC. What happens next is that I build a MEM_REF of and ADDR_EXPR of a handled_component (which may possibly be of a MEM_REF) which is bogus on its own but it is handled well by the subsequent call to force_gimple_operand (I hadle it myself in the actual reimplementation of build_ref_for_offsets because there I would not call force_gimple_operand anyway). > > > + { > > + base = build_fold_addr_expr (prev_base); > > + off = build_int_cst (TREE_TYPE (base), > > + adj->offset / BITS_PER_UNIT); > > + } > > + else if (TREE_CODE (base) == MEM_REF) > > + { > > + off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)), > > + base_offset > > + + adj->offset / BITS_PER_UNIT); > > + off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), > > + off, 0); > > + base = TREE_OPERAND (base, 0); > > + } > > + else > > + { > > + off = build_int_cst (build_pointer_type (TREE_TYPE (base)), > > + base_offset > > + + adj->offset / BITS_PER_UNIT); > > + base = build_fold_addr_expr (base); > > You are choosing some random types for the offset. Please try > to preserve that of the original access. (It's not clear to me what > we are replacing with what here ...) > > See tree.c:reference_alias_ptr_type for a way to get the alias > type of an existing reference. Well, I don't really understand what you mean by random here. It is true that I don't go for the TYPE_MAIN_VARIANT, is that the problem? I have changed the patch a bit to actually use reference_alias_ptr_type. If the original argument came from a MEM_REF, I do preserve the type from it. If we have a base decl, I use a pointer to it (or now its main variant). Only when I just have a pointer I simply trust its type. Is that wrong now, do I somehow have to propagate the reference type from the callee to the caller here? > > > + } > > } > > - else > > + > > + expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off); > > + if (adj->by_ref) > > + expr = build_fold_addr_expr (expr); > > Separating the by_ref (or allow_ptr?!) cases could make things > easier to understand and avoid stripping off and re-building ADDR_EXPRs > all the time? With this patch there is no allow_ptr any more, all the other callers pass false for this parameter to build_ref_for_offset - moreover, allow_ptr simply allowed the original parameter - the one which is being transformed into a new one - to be passed by reference (and it was more of a sanity check anyway. On the contrary, when adj->by_ref is true, it means that the newly created parameter should be passed by reference. We make it such by simply encapsulating the reference into an aggregate in an ADDR_EXPR. Building of the reference would be exactly the same and so I don't think separating the case would help to streamline this. > > I have a hard time extracting patches from google-mail, so if you > can bounce me the patch to my suse address I can have a look > at the expr.c hunk. I believe that in gmail, in the menu next to the reply button, there is a "Show original" command that can help with extracting patches. Anyway, thanks for looking into this. Below is a context diff of the slightly amended version which might be more readable and I will also attach unified diff. I'll also CC your suse address. Martin 2010-08-04 Martin Jambor * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of calling build_ref_for_offset. * expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also when sizes don't match but expected mode is BLKmode. * testsuite/g++.dg/torture/pr34850.C: Remove expected warning. Index: mine/gcc/ipa-prop.c =================================================================== *** mine.orig/gcc/ipa-prop.c --- mine/gcc/ipa-prop.c *************** ipa_modify_call_arguments (struct cgraph *** 2149,2188 **** } else if (!adj->remove_param) { ! tree expr, orig_expr; ! bool allow_ptr, repl_found; ! orig_expr = expr = gimple_call_arg (stmt, adj->base_index); ! if (TREE_CODE (expr) == ADDR_EXPR) ! { ! allow_ptr = false; ! expr = TREE_OPERAND (expr, 0); ! } ! else ! allow_ptr = true; ! repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr), ! adj->offset, adj->type, ! allow_ptr); ! if (repl_found) { ! if (adj->by_ref) ! expr = build_fold_addr_expr (expr); } ! else { ! tree ptrtype = build_pointer_type (adj->type); ! expr = orig_expr; ! if (!POINTER_TYPE_P (TREE_TYPE (expr))) ! expr = build_fold_addr_expr (expr); ! if (!useless_type_conversion_p (ptrtype, TREE_TYPE (expr))) ! expr = fold_convert (ptrtype, expr); ! expr = fold_build2 (POINTER_PLUS_EXPR, ptrtype, expr, ! build_int_cst (sizetype, ! adj->offset / BITS_PER_UNIT)); ! if (!adj->by_ref) ! expr = fold_build1 (INDIRECT_REF, adj->type, expr); } expr = force_gimple_operand_gsi (&gsi, expr, adj->by_ref || is_gimple_reg_type (adj->type), --- 2149,2232 ---- } else if (!adj->remove_param) { ! tree expr, base, off; ! location_t loc; ! /* We create a new parameter out of the value of the old one, we can ! do the following kind of transformations: ! ! - A scalar passed by reference is converted to a scalar passed by ! value. (adj->by_ref is false and the type of the original ! actual argument is a pointer to a scalar). ! ! - A part of an aggregate is passed instead of the whole aggregate. ! The part can be passed either by value or by reference, this is ! determined by value of adj->by_ref. Moreover, the code below ! handles both situations when the original aggregate is passed by ! value (its type is not a pointer) and when it is passed by ! reference (it is a pointer to an aggregate). ! ! When the new argument is passed by reference (adj->by_ref is true) ! it must be a part of an aggregate and therefore we form it by ! simply taking the address of a reference inside the original ! aggregate. */ ! ! gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0); ! base = gimple_call_arg (stmt, adj->base_index); ! loc = EXPR_LOCATION (base); ! if (TREE_CODE (base) == ADDR_EXPR ! && DECL_P (TREE_OPERAND (base, 0))) ! off = build_int_cst (reference_alias_ptr_type (base), ! adj->offset / BITS_PER_UNIT); ! else if (TREE_CODE (base) != ADDR_EXPR ! && POINTER_TYPE_P (TREE_TYPE (base))) ! off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT); ! else { ! HOST_WIDE_INT base_offset; ! tree prev_base; ! ! if (TREE_CODE (base) == ADDR_EXPR) ! base = TREE_OPERAND (base, 0); ! prev_base = base; ! base = get_addr_base_and_unit_offset (base, &base_offset); ! if (!base) ! { ! base = build_fold_addr_expr (prev_base); ! off = build_int_cst (reference_alias_ptr_type (prev_base), ! adj->offset / BITS_PER_UNIT); ! } ! else if (TREE_CODE (base) == MEM_REF) ! { ! off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)), ! base_offset ! + adj->offset / BITS_PER_UNIT); ! off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), ! off, 0); ! base = TREE_OPERAND (base, 0); ! } ! else ! { ! off = build_int_cst (reference_alias_ptr_type (base), ! base_offset ! + adj->offset / BITS_PER_UNIT); ! base = build_fold_addr_expr (base); ! } } ! ! expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off); ! if (adj->by_ref) ! expr = build_fold_addr_expr (expr); ! ! /* !!! Remove after testing */ ! if (dump_file) { ! fprintf (dump_file, "Built MEM_REF: "); ! print_generic_expr (dump_file, expr, 0); ! fprintf (dump_file, "\n"); } + expr = force_gimple_operand_gsi (&gsi, expr, adj->by_ref || is_gimple_reg_type (adj->type), Index: mine/gcc/expr.c =================================================================== *** mine.orig/gcc/expr.c --- mine/gcc/expr.c *************** expand_expr_real_1 (tree exp, rtx target *** 8709,8716 **** tree bftype; if (offset == 0 && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1) ! && (GET_MODE_BITSIZE (DECL_MODE (base)) ! == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))) return expand_expr (build1 (VIEW_CONVERT_EXPR, TREE_TYPE (exp), base), target, tmode, modifier); --- 8709,8719 ---- tree bftype; if (offset == 0 && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1) ! && ((GET_MODE_BITSIZE (DECL_MODE (base)) ! == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))) ! || (TYPE_MODE (TREE_TYPE (exp)) == BLKmode ! && (GET_MODE_BITSIZE (DECL_MODE (base)) ! >= TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))))) return expand_expr (build1 (VIEW_CONVERT_EXPR, TREE_TYPE (exp), base), target, tmode, modifier); Index: mine/gcc/testsuite/g++.dg/torture/pr34850.C =================================================================== *** mine.orig/gcc/testsuite/g++.dg/torture/pr34850.C --- mine/gcc/testsuite/g++.dg/torture/pr34850.C *************** extern "C" { *** 11,17 **** extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__)) void * memset (void *__dest, int __ch, size_t __len) throw () { if (__builtin_constant_p (__len) && __len == 0) ! __warn_memset_zero_len (); /* { dg-warning "" } */ } } inline void clear_mem(void* ptr, u32bit n) { --- 11,17 ---- extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__)) void * memset (void *__dest, int __ch, size_t __len) throw () { if (__builtin_constant_p (__len) && __len == 0) ! __warn_memset_zero_len (); } } inline void clear_mem(void* ptr, u32bit n) { 2010-08-04 Martin Jambor * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of calling build_ref_for_offset. * expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also when sizes don't match but expected mode is BLKmode. * testsuite/g++.dg/torture/pr34850.C: Remove expected warning. Index: mine/gcc/ipa-prop.c =================================================================== --- mine.orig/gcc/ipa-prop.c +++ mine/gcc/ipa-prop.c @@ -2149,40 +2149,84 @@ ipa_modify_call_arguments (struct cgraph } else if (!adj->remove_param) { - tree expr, orig_expr; - bool allow_ptr, repl_found; + tree expr, base, off; + location_t loc; - orig_expr = expr = gimple_call_arg (stmt, adj->base_index); - if (TREE_CODE (expr) == ADDR_EXPR) - { - allow_ptr = false; - expr = TREE_OPERAND (expr, 0); - } - else - allow_ptr = true; + /* We create a new parameter out of the value of the old one, we can + do the following kind of transformations: + + - A scalar passed by reference is converted to a scalar passed by + value. (adj->by_ref is false and the type of the original + actual argument is a pointer to a scalar). + + - A part of an aggregate is passed instead of the whole aggregate. + The part can be passed either by value or by reference, this is + determined by value of adj->by_ref. Moreover, the code below + handles both situations when the original aggregate is passed by + value (its type is not a pointer) and when it is passed by + reference (it is a pointer to an aggregate). + + When the new argument is passed by reference (adj->by_ref is true) + it must be a part of an aggregate and therefore we form it by + simply taking the address of a reference inside the original + aggregate. */ + + gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0); + base = gimple_call_arg (stmt, adj->base_index); + loc = EXPR_LOCATION (base); - repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr), - adj->offset, adj->type, - allow_ptr); - if (repl_found) + if (TREE_CODE (base) == ADDR_EXPR + && DECL_P (TREE_OPERAND (base, 0))) + off = build_int_cst (reference_alias_ptr_type (base), + adj->offset / BITS_PER_UNIT); + else if (TREE_CODE (base) != ADDR_EXPR + && POINTER_TYPE_P (TREE_TYPE (base))) + off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT); + else { - if (adj->by_ref) - expr = build_fold_addr_expr (expr); + HOST_WIDE_INT base_offset; + tree prev_base; + + if (TREE_CODE (base) == ADDR_EXPR) + base = TREE_OPERAND (base, 0); + prev_base = base; + base = get_addr_base_and_unit_offset (base, &base_offset); + if (!base) + { + base = build_fold_addr_expr (prev_base); + off = build_int_cst (reference_alias_ptr_type (prev_base), + adj->offset / BITS_PER_UNIT); + } + else if (TREE_CODE (base) == MEM_REF) + { + off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)), + base_offset + + adj->offset / BITS_PER_UNIT); + off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), + off, 0); + base = TREE_OPERAND (base, 0); + } + else + { + off = build_int_cst (reference_alias_ptr_type (base), + base_offset + + adj->offset / BITS_PER_UNIT); + base = build_fold_addr_expr (base); + } } - else + + expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off); + if (adj->by_ref) + expr = build_fold_addr_expr (expr); + + /* !!! Remove after testing */ + if (dump_file) { - tree ptrtype = build_pointer_type (adj->type); - expr = orig_expr; - if (!POINTER_TYPE_P (TREE_TYPE (expr))) - expr = build_fold_addr_expr (expr); - if (!useless_type_conversion_p (ptrtype, TREE_TYPE (expr))) - expr = fold_convert (ptrtype, expr); - expr = fold_build2 (POINTER_PLUS_EXPR, ptrtype, expr, - build_int_cst (sizetype, - adj->offset / BITS_PER_UNIT)); - if (!adj->by_ref) - expr = fold_build1 (INDIRECT_REF, adj->type, expr); + fprintf (dump_file, "Built MEM_REF: "); + print_generic_expr (dump_file, expr, 0); + fprintf (dump_file, "\n"); } + expr = force_gimple_operand_gsi (&gsi, expr, adj->by_ref || is_gimple_reg_type (adj->type), Index: mine/gcc/expr.c =================================================================== --- mine.orig/gcc/expr.c +++ mine/gcc/expr.c @@ -8709,8 +8709,11 @@ expand_expr_real_1 (tree exp, rtx target tree bftype; if (offset == 0 && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1) - && (GET_MODE_BITSIZE (DECL_MODE (base)) - == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))) + && ((GET_MODE_BITSIZE (DECL_MODE (base)) + == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))) + || (TYPE_MODE (TREE_TYPE (exp)) == BLKmode + && (GET_MODE_BITSIZE (DECL_MODE (base)) + >= TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))))) return expand_expr (build1 (VIEW_CONVERT_EXPR, TREE_TYPE (exp), base), target, tmode, modifier); Index: mine/gcc/testsuite/g++.dg/torture/pr34850.C =================================================================== --- mine.orig/gcc/testsuite/g++.dg/torture/pr34850.C +++ mine/gcc/testsuite/g++.dg/torture/pr34850.C @@ -11,7 +11,7 @@ extern "C" { extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__)) void * memset (void *__dest, int __ch, size_t __len) throw () { if (__builtin_constant_p (__len) && __len == 0) - __warn_memset_zero_len (); /* { dg-warning "" } */ + __warn_memset_zero_len (); } } inline void clear_mem(void* ptr, u32bit n) {