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 <[email protected]> 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 <[email protected]>
>
> 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" } } */
+
