From patchwork Thu Sep 29 08:27:02 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 116923 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 CD1E4B6F8D for ; Thu, 29 Sep 2011 18:25:52 +1000 (EST) Received: (qmail 12326 invoked by alias); 29 Sep 2011 08:25:44 -0000 Received: (qmail 12284 invoked by uid 22791); 29 Sep 2011 08:25:37 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,TW_CP X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 29 Sep 2011 08:25:16 +0000 Received: from nat-dem.mentorg.com ([195.212.93.2] helo=eu2-mail.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1R9Bve-0006iE-Vk from Tom_deVries@mentor.com ; Thu, 29 Sep 2011 01:25:15 -0700 Received: from [127.0.0.1] ([172.16.63.104]) by eu2-mail.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Thu, 29 Sep 2011 10:25:13 +0200 Message-ID: <4E842BD6.9030102@mentor.com> Date: Thu, 29 Sep 2011 10:27:02 +0200 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13 MIME-Version: 1.0 To: Richard Guenther CC: Tom de Vries , "gcc-patches@gcc.gnu.org" , Maxim Kuvyrkov Subject: Re: [PATCH, PR43814] Assume function arguments of pointer type are aligned. References: <4E787543.1090009@codesourcery.com> <4E839DE5.4070709@mentor.com> In-Reply-To: <4E839DE5.4070709@mentor.com> 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 On 09/29/2011 12:21 AM, Tom de Vries wrote: > On 09/24/2011 11:31 AM, Richard Guenther wrote: >> On Tue, Sep 20, 2011 at 1:13 PM, Tom de Vries wrote: >>> Hi Richard, >>> >>> I have a patch for PR43814. It introduces an option that assumes that function >>> arguments of pointer type are aligned, and uses that information in >>> tree-ssa-ccp. This enables the memcpy in pr43814-2.c to be inlined. >>> >>> I tested the patch successfully on-by-default on x86_64 and i686 (both gcc only >>> builds). >>> >>> I also tested the patch on-by-default for ARM (gcc/glibc build). The patch >>> generated wrong code for uselocale.c: >>> ... >>> glibc/locale/locale.h: >>> ... >>> /* This value can be passed to `uselocale' and may be returned by >>> it. Passing this value to any other function has undefined behavior. */ >>> # define LC_GLOBAL_LOCALE ((__locale_t) -1L) >>> ... >>> glibc/locale/uselocale.c: >>> ... >>> locale_t >>> __uselocale (locale_t newloc) >>> { >>> locale_t oldloc = _NL_CURRENT_LOCALE; >>> >>> if (newloc != NULL) >>> { >>> const locale_t locobj >>> = newloc == LC_GLOBAL_LOCALE ? &_nl_global_locale : newloc; >>> >>> ... >>> The assumption that function arguments of pointer type are aligned, allowed the >>> test 'newloc == LC_GLOBAL_LOCALE' to evaluate to false. >>> But the usage of ((__locale_t) -1L) as function argument in uselocale violates >>> that assumption. >>> >>> Fixing the definition of LC_GLOBAL_LOCALE allowed the gcc tests to run without >>> regressions for ARM. >>> >>> Furthermore, the patch fixes ipa-sra-2.c and ipa-sra-6.c regressions on ARM, >>> discussed here: >>> - http://gcc.gnu.org/ml/gcc-patches/2011-08/msg00930.html >>> - http://gcc.gnu.org/ml/gcc-patches/2011-09/msg00459.html >>> >>> But, since glibc uses this construct currently, the option is off-by-default for >>> now. >>> >>> OK for trunk? >> >> No, I don't like to have an option to control this. And given the issue >> you spotted it doesn't look like the best idea either. This area in GCC >> is simply too fragile right now :/ >> >> It would be nice if we could extend IPA-CP to propagate alignment >> information though. >> >> And at some point devise a reliable way for frontends to communicate >> alignment constraints the middle-end can rely on (well, yes, you could >> argue that's what TYPE_ALIGN is about, and I thought that maybe we >> can unconditionally rely on TYPE_ALIGN for pointer PARM_DECLs >> at least - your example shows we can't). >> >> In the end I'd probably say the patch is ok without the option (thus >> turned on by default), but if LC_GLOBAL_LOCALE is part of the >> glibc ABI then we clearly can't do this. >> > > I removed the flag, and enabled the optimization now with the aligned attribute. > > bootstrapped and tested on x86_64 (gcc only build), and build and reg-tested on > arm (gcc + glibc build), no issues found. > Sorry for the EWRONGPATCH. Correct patch this time. OK for trunk? Thanks, - Tom > > I intend to send a follow-up patch that introduces a target hook > function_pointers_aligned, that is false by default, and on by default for > -mandroid. I asked Google to test their Android codebase with the optimization > on by default. Would such a target hook be acceptable? > >> Richard. >> > > Thanks, > - Tom > > 2011-09-29 Tom de Vries > > PR target/43814 > * tree-ssa-ccp.c (get_align_value): New function, factored out of > get_value_from_alignment. > (get_value_from_alignment): Use get_align_value. > (get_value_for_expr): Use get_align_value to handle alignment of > function argument pointers with TYPE_USER_ALIGN. > > * gcc/testsuite/gcc.dg/pr43814.c: New test. > * gcc/testsuite/gcc.target/arm/pr43814-2.c: New test. Index: gcc/tree-ssa-ccp.c =================================================================== --- gcc/tree-ssa-ccp.c (revision 179210) +++ gcc/tree-ssa-ccp.c (working copy) @@ -500,20 +500,14 @@ value_to_double_int (prop_value_t val) return double_int_zero; } -/* Return the value for the address expression EXPR based on alignment - information. */ +/* Return the value for an expr of type TYPE with alignment ALIGN and offset + BITPOS relative to the alignment. */ static prop_value_t -get_value_from_alignment (tree expr) +get_align_value (unsigned int align, tree type, unsigned HOST_WIDE_INT bitpos) { - tree type = TREE_TYPE (expr); prop_value_t val; - unsigned HOST_WIDE_INT bitpos; - unsigned int align; - - gcc_assert (TREE_CODE (expr) == ADDR_EXPR); - align = get_object_alignment_1 (TREE_OPERAND (expr, 0), &bitpos); val.mask = double_int_and_not (POINTER_TYPE_P (type) || TYPE_UNSIGNED (type) ? double_int_mask (TYPE_PRECISION (type)) @@ -529,6 +523,21 @@ get_value_from_alignment (tree expr) return val; } +/* Return the value for the address expression EXPR based on alignment + information. */ + +static prop_value_t +get_value_from_alignment (tree expr) +{ + unsigned int align; + unsigned HOST_WIDE_INT bitpos; + + gcc_assert (TREE_CODE (expr) == ADDR_EXPR); + + align = get_object_alignment_1 (TREE_OPERAND (expr, 0), &bitpos); + return get_align_value (align, TREE_TYPE (expr), bitpos); +} + /* Return the value for the tree operand EXPR. If FOR_BITS_P is true return constant bits extracted from alignment information for invariant addresses. */ @@ -540,11 +549,21 @@ get_value_for_expr (tree expr, bool for_ if (TREE_CODE (expr) == SSA_NAME) { + tree var = SSA_NAME_VAR (expr); val = *get_value (expr); - if (for_bits_p - && val.lattice_val == CONSTANT + if (!for_bits_p) + return val; + + if (val.lattice_val == CONSTANT && TREE_CODE (val.value) == ADDR_EXPR) val = get_value_from_alignment (val.value); + else if (val.lattice_val == VARYING + && SSA_NAME_IS_DEFAULT_DEF (expr) + && TREE_CODE (var) == PARM_DECL + && POINTER_TYPE_P (TREE_TYPE (var)) + && TYPE_USER_ALIGN (TREE_TYPE (TREE_TYPE (var)))) + val = get_align_value (TYPE_ALIGN (TREE_TYPE (TREE_TYPE (var))), + TREE_TYPE (var), 0); } else if (is_gimple_min_invariant (expr) && (!for_bits_p || TREE_CODE (expr) != ADDR_EXPR)) Index: gcc/testsuite/gcc.dg/pr43814.c =================================================================== --- /dev/null (new file) +++ gcc/testsuite/gcc.dg/pr43814.c (revision 0) @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-ccp1-all" } */ + +typedef __attribute__((aligned (sizeof (int)))) int aint; + +void +f (aint *a) +{ + *(a+1) = 0; +} + +/* { dg-final { scan-tree-dump-times "ALIGN = \[2-9\]\[0-9\]*, MISALIGN = 0" 1 "ccp1"} } */ +/* { dg-final { cleanup-tree-dump "ccp1" } } */ + Index: gcc/testsuite/gcc.target/arm/pr43814-2.c =================================================================== --- /dev/null (new file) +++ gcc/testsuite/gcc.target/arm/pr43814-2.c (revision 0) @@ -0,0 +1,36 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-rtl-expand" } */ + +typedef unsigned int size_t; +extern void* memcpy (void *, const void *, size_t); + +typedef __attribute__((aligned (sizeof (int)))) + const unsigned int cst_aligned_uint; + +typedef union JValue { + void* l; +} JValue; +typedef struct Object { + int x; +} Object; + +extern __inline__ long long +dvmGetArgLong (cst_aligned_uint* args, int elem) +{ + long long val; + memcpy (&val, &args[elem], 8); + return val; +} + +void +Dalvik_sun_misc_Unsafe_getObject (cst_aligned_uint* args, JValue* pResult) +{ + Object* obj = (Object*) args[1]; + long long offset = dvmGetArgLong (args, 2); + Object** address = (Object**) (((unsigned char*) obj) + offset); + pResult->l = ((void*) *address); +} + +/* { dg-final { scan-rtl-dump-times "memcpy" 0 "expand"} } */ +/* { dg-final { cleanup-tree-dump "expand" } } */ +