On Thu, Sep 29, 2011 at 10:27 AM, Tom de Vries <tom_devr...@mentor.com> wrote: > 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 <vr...@codesourcery.com> >>> 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?
+ else if (val.lattice_val == VARYING With default-def PARM_DECLs this should be always true, so no need to check it. + && 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); I realize that the scope where this applies is pretty narrow (and thus bad fallout is quite unlikely). But I suppose we should document somewhere that GCC treats alignment attributes on parameters more strict than say, on assignments. Btw, for this particular case Jakub invented __builtin_assume_aligned, which I think is strictly superior. So I'm not sure we should promote the "old way" which isn't very well supported. Thus, I believe frontends should insert such builtin calls when they think it is safe to do, I don't like the middle-end extracting too much alignment information from types - an area that's very gray in GCC. I'm leaving this for comments from others for now. Thanks, Richard. > 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 <t...@codesourcery.com> >> >> 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. > >