[PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

Hi!

This is an attempt to fix the following testcase (reduced from gsoap)
similarly how you've fixed another issue with r221795 other AAPCS
regressions introduced with r221348 change.
This patch passed bootstrap/regtest on
{x86_64,i686,armv7hl,aarch64,powerpc64{,le},s390{,x}}-linux.

Though, it still doesn't fix profiledbootstrap on armv7hl that is broken
since r221348, so other issues are lurking in there, and I must say
I'm not entirely sure about this, because it changes alignment even when
the original access had higher alignment.

I was trying something like:
struct B { char *a, *b; };
typedef struct B C __attribute__((aligned (8)));
struct A { C a; int b; long long c; };
char v[3];

__attribute__((noinline, noclone)) void
fn1 (C x, C y)
{
  if (x.a != &v[1] || y.a != &v[2])
    __builtin_abort ();
  v[1]++;
}

__attribute__((noinline, noclone)) int
fn2 (C x)
{
  asm volatile ("" : "+g" (x.a) : : "memory");
  asm volatile ("" : "+g" (x.b) : : "memory");
  return x.a == &v[0];
}

__attribute__((noinline, noclone)) void
fn3 (const char *x)
{
  if (x[0] != 0)
    __builtin_abort ();
}

static struct A
foo (const char *x, struct A y, struct A z)
{
  struct A r = { { 0, 0 }, 0, 0 };
  if (y.b && z.b)
    {
      if (fn2 (y.a) && fn2 (z.a))
	switch (x[0])
	  {
	  case '|':
	    break;
	  default:
	    fn3 (x);
	  }
      fn1 (y.a, z.a);
    }
  return r;
}

__attribute__((noinline, noclone)) int
bar (int x, struct A *y)
{
  switch (x)
    {
    case 219:
      foo ("+", y[-2], y[0]);
    case 220:
      foo ("-", y[-2], y[0]);
    }
}

int
main ()
{
  struct A a[3] = { { { &v[1], &v[0] }, 1, 1LL },
		    { { &v[0], &v[0] }, 0, 0LL },
		    { { &v[2], &v[0] }, 2, 2LL } };
  bar (220, a + 2);
  if (v[1] != 1)
    __builtin_abort ();
  return 0;
}

and this patch indeed changes the register passing, eventhough it probably
shouldn't (though, the testcase doesn't fail).  Wouldn't it be possible to
preserve the original type (before we call build_aligned_type on it)
somewhere in SRA data structures, perhaps keep expr (the new MEM_REF) use
the aligned type, but type field be the non-aligned one?

2015-05-02  Jakub Jelinek  <jakub@redhat.com>

	PR target/65956
	* tree-sra.c (turn_representatives_into_adjustments): For
	adj.type, use TYPE_MAIN_VARIANT of repr->type with TYPE_QUALS.

	* gcc.c-torture/execute/pr65956.c: New test.

--- gcc/tree-sra.c.jj	2015-04-20 14:35:47.000000000 +0200
+++ gcc/tree-sra.c	2015-05-01 01:08:34.092636496 +0200
@@ -4427,7 +4427,11 @@ turn_representatives_into_adjustments (v
 	      gcc_assert (repr->base == parm);
 	      adj.base_index = index;
 	      adj.base = repr->base;
-	      adj.type = repr->type;
+	      /* Drop any special alignment on the type if it's not on the
+		 main variant.  This avoids issues with weirdo ABIs like
+		 AAPCS.  */
+	      adj.type = build_qualified_type (TYPE_MAIN_VARIANT (repr->type),
+					       TYPE_QUALS (repr->type));
 	      adj.alias_ptr_type = reference_alias_ptr_type (repr->expr);
 	      adj.offset = repr->offset;
 	      adj.by_ref = (POINTER_TYPE_P (TREE_TYPE (repr->base))
--- gcc/testsuite/gcc.c-torture/execute/pr65956.c.jj	2015-05-01 10:32:34.730150257 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr65956.c	2015-05-01 10:32:13.000000000 +0200
@@ -0,0 +1,67 @@
+/* PR target/65956 */
+
+struct A { char *a; int b; long long c; };
+char v[3];
+
+__attribute__((noinline, noclone)) void
+fn1 (char *x, char *y)
+{
+  if (x != &v[1] || y != &v[2])
+    __builtin_abort ();
+  v[1]++;
+}
+
+__attribute__((noinline, noclone)) int
+fn2 (char *x)
+{
+  asm volatile ("" : "+g" (x) : : "memory");
+  return x == &v[0];
+}
+
+__attribute__((noinline, noclone)) void
+fn3 (const char *x)
+{
+  if (x[0] != 0)
+    __builtin_abort ();
+}
+
+static struct A
+foo (const char *x, struct A y, struct A z)
+{
+  struct A r = { 0, 0, 0 };
+  if (y.b && z.b)
+    {
+      if (fn2 (y.a) && fn2 (z.a))
+	switch (x[0])
+	  {
+	  case '|':
+	    break;
+	  default:
+	    fn3 (x);
+	  }
+      fn1 (y.a, z.a);
+    }
+  return r;
+}
+
+__attribute__((noinline, noclone)) int
+bar (int x, struct A *y)
+{
+  switch (x)
+    {
+    case 219:
+      foo ("+", y[-2], y[0]);
+    case 220:
+      foo ("-", y[-2], y[0]);
+    }
+}
+
+int
+main ()
+{
+  struct A a[3] = { { &v[1], 1, 1LL }, { &v[0], 0, 0LL }, { &v[2], 2, 2LL } };
+  bar (220, a + 2);
+  if (v[1] != 1)
+    __builtin_abort ();
+  return 0;
+}

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Biener]

On Sat, 2 May 2015, Jakub Jelinek wrote:

> Hi!
> 
> This is an attempt to fix the following testcase (reduced from gsoap)
> similarly how you've fixed another issue with r221795 other AAPCS
> regressions introduced with r221348 change.
> This patch passed bootstrap/regtest on
> {x86_64,i686,armv7hl,aarch64,powerpc64{,le},s390{,x}}-linux.
> 
> Though, it still doesn't fix profiledbootstrap on armv7hl that is broken
> since r221348, so other issues are lurking in there, and I must say
> I'm not entirely sure about this, because it changes alignment even when
> the original access had higher alignment.
> 
> I was trying something like:
> struct B { char *a, *b; };
> typedef struct B C __attribute__((aligned (8)));
> struct A { C a; int b; long long c; };
> char v[3];
> 
> __attribute__((noinline, noclone)) void
> fn1 (C x, C y)
> {
>   if (x.a != &v[1] || y.a != &v[2])
>     __builtin_abort ();
>   v[1]++;
> }
> 
> __attribute__((noinline, noclone)) int
> fn2 (C x)
> {
>   asm volatile ("" : "+g" (x.a) : : "memory");
>   asm volatile ("" : "+g" (x.b) : : "memory");
>   return x.a == &v[0];
> }
> 
> __attribute__((noinline, noclone)) void
> fn3 (const char *x)
> {
>   if (x[0] != 0)
>     __builtin_abort ();
> }
> 
> static struct A
> foo (const char *x, struct A y, struct A z)
> {
>   struct A r = { { 0, 0 }, 0, 0 };
>   if (y.b && z.b)
>     {
>       if (fn2 (y.a) && fn2 (z.a))
> 	switch (x[0])
> 	  {
> 	  case '|':
> 	    break;
> 	  default:
> 	    fn3 (x);
> 	  }
>       fn1 (y.a, z.a);
>     }
>   return r;
> }
> 
> __attribute__((noinline, noclone)) int
> bar (int x, struct A *y)
> {
>   switch (x)
>     {
>     case 219:
>       foo ("+", y[-2], y[0]);
>     case 220:
>       foo ("-", y[-2], y[0]);
>     }
> }
> 
> int
> main ()
> {
>   struct A a[3] = { { { &v[1], &v[0] }, 1, 1LL },
> 		    { { &v[0], &v[0] }, 0, 0LL },
> 		    { { &v[2], &v[0] }, 2, 2LL } };
>   bar (220, a + 2);
>   if (v[1] != 1)
>     __builtin_abort ();
>   return 0;
> }
> 
> and this patch indeed changes the register passing, eventhough it probably
> shouldn't (though, the testcase doesn't fail).  Wouldn't it be possible to
> preserve the original type (before we call build_aligned_type on it)
> somewhere in SRA data structures, perhaps keep expr (the new MEM_REF) use
> the aligned type, but type field be the non-aligned one?

Not sure how this helps when SRA tears apart the parameter.  That is,
isn't the important thing that both the IPA modified function argument
types/decls have the same type as the types of the parameters SRA ends
up passing?  (as far as alignment goes?)

Yes, of course using "natural" alignment makes sure that the backend
can handle alignment properly and we don't run into oddball bugs here.

> 2015-05-02  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/65956
> 	* tree-sra.c (turn_representatives_into_adjustments): For
> 	adj.type, use TYPE_MAIN_VARIANT of repr->type with TYPE_QUALS.
> 
> 	* gcc.c-torture/execute/pr65956.c: New test.
> 
> --- gcc/tree-sra.c.jj	2015-04-20 14:35:47.000000000 +0200
> +++ gcc/tree-sra.c	2015-05-01 01:08:34.092636496 +0200
> @@ -4427,7 +4427,11 @@ turn_representatives_into_adjustments (v
>  	      gcc_assert (repr->base == parm);
>  	      adj.base_index = index;
>  	      adj.base = repr->base;
> -	      adj.type = repr->type;
> +	      /* Drop any special alignment on the type if it's not on the
> +		 main variant.  This avoids issues with weirdo ABIs like
> +		 AAPCS.  */
> +	      adj.type = build_qualified_type (TYPE_MAIN_VARIANT (repr->type),
> +					       TYPE_QUALS (repr->type));

So - this changes the function argument type of the clone?  Does it
also change the type of the value we pass to the function?  That is,
why drop the alignment here but not avoid attaching it to repr->type
in the first place as my fix for the other issue did?

Doesn't the above just make it inconsistent by default?

There is also the correctness issue of under-aligned types (which
was what the original code using build_aligned_type cared for - before
I "fixed" it to also preserve over-alignment).

That said - somewhere we create the register we use for passing the
argument, and only the type of that register needs fixing IMHO.

We also have

              ptype = adj->type;
              if (is_gimple_reg_type (ptype))
                {
                  unsigned malign = GET_MODE_ALIGNMENT (TYPE_MODE 
(ptype));
                  if (TYPE_ALIGN (ptype) < malign)
                    ptype = build_aligned_type (ptype, malign);

in ipa_modify_formal_parameters.  That looks odd for by-value passing
as well.  When modifying the function bodies we simply take what was
set in ->new_decl which we'd populate above in 
ipa_modify_formal_parameters.  It seems to me that ipa_modify_expr
should look to preserve alignment at the callers site (for loading
into the regs we pass) for non-reference passing.  Esp.

  if (cand->by_ref)
    src = build_simple_mem_ref (cand->new_decl);

looks bogus in this regard (unless we make sure that the pointed-to
type of cand->new_decl has all under-/over-alignment applied properly).

It would probably be clearer to record "alignment of original memory
access" in the data structures and apply that only to memory references
we generate, not to registers we eventually end up creating.

To me all of that IPA SRA stuff is a bit of a twisted maze ... but
we definitely want to root out any use of over-/under-alignment we
generate for PARM_DECLs and decls/SSA names we end up passing by
value.

I'm not 100% sure your patch walks in that direction without changing
alignment of memory references we eventually generate.

But maybe Martin can shed some light on this...

Thanks,
Richard.


>  	      adj.alias_ptr_type = reference_alias_ptr_type (repr->expr);
>  	      adj.offset = repr->offset;
>  	      adj.by_ref = (POINTER_TYPE_P (TREE_TYPE (repr->base))
> --- gcc/testsuite/gcc.c-torture/execute/pr65956.c.jj	2015-05-01 10:32:34.730150257 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr65956.c	2015-05-01 10:32:13.000000000 +0200
> @@ -0,0 +1,67 @@
> +/* PR target/65956 */
> +
> +struct A { char *a; int b; long long c; };
> +char v[3];
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (char *x, char *y)
> +{
> +  if (x != &v[1] || y != &v[2])
> +    __builtin_abort ();
> +  v[1]++;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn2 (char *x)
> +{
> +  asm volatile ("" : "+g" (x) : : "memory");
> +  return x == &v[0];
> +}
> +
> +__attribute__((noinline, noclone)) void
> +fn3 (const char *x)
> +{
> +  if (x[0] != 0)
> +    __builtin_abort ();
> +}
> +
> +static struct A
> +foo (const char *x, struct A y, struct A z)
> +{
> +  struct A r = { 0, 0, 0 };
> +  if (y.b && z.b)
> +    {
> +      if (fn2 (y.a) && fn2 (z.a))
> +	switch (x[0])
> +	  {
> +	  case '|':
> +	    break;
> +	  default:
> +	    fn3 (x);
> +	  }
> +      fn1 (y.a, z.a);
> +    }
> +  return r;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +bar (int x, struct A *y)
> +{
> +  switch (x)
> +    {
> +    case 219:
> +      foo ("+", y[-2], y[0]);
> +    case 220:
> +      foo ("-", y[-2], y[0]);
> +    }
> +}
> +
> +int
> +main ()
> +{
> +  struct A a[3] = { { &v[1], 1, 1LL }, { &v[0], 0, 0LL }, { &v[2], 2, 2LL } };
> +  bar (220, a + 2);
> +  if (v[1] != 1)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Mon, May 04, 2015 at 10:11:13AM +0200, Richard Biener wrote:
> Not sure how this helps when SRA tears apart the parameter.  That is,
> isn't the important thing that both the IPA modified function argument
> types/decls have the same type as the types of the parameters SRA ends
> up passing?  (as far as alignment goes?)
> 
> Yes, of course using "natural" alignment makes sure that the backend
> can handle alignment properly and we don't run into oddball bugs here.

On IRC we were discussing making

 /* Return true if mode/type need doubleword alignment.  */
 static bool
 arm_needs_doubleword_align (machine_mode mode, const_tree type)
 {
   return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
-	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
+	  || (type && TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY));
 }


Looking at

struct S { char a[16]; }; 
typedef struct S T;
typedef struct S U __attribute__((aligned (16))); 
struct V { U u; T v; };
typedef int N __attribute__((aligned (16)));

T t1;
U u1;
int a[3];

void
f5 (__builtin_va_list *ap)
{
  t1 = __builtin_va_arg (*ap, T);
  a[0] = __builtin_va_arg (*ap, int);
  u1 = __builtin_va_arg (*ap, U);
  a[1] = __builtin_va_arg (*ap, int);
  a[2] = __builtin_va_arg (*ap, N);
}

void f6 (int, N, int, U);

void
f7 (void)
{
  U u = {};
  f6 (0, (N) 1, 0, u);
}

and s/16/8/g output, it seems that neither i?86 nor x86_64 care about
the alignment for any passing, ppc64le cares about aggregates, but not
scalars apparently (with a warning that the passing changed), arm cares
about both.  And the f7 function shows that for non-aggregates, what arm
does is simply never going to work, because there is no way to pass down
the scalars aligned, f6 is still called with 1 in int type rather than N.

So at least changing arm_needs_doubleword_align for non-aggregates would
likely not break anything that hasn't been broken already and would unbreak
the majority of cases.

The following testcase shows that eipa_sra changes alignment even for the
aggregates.  Change aligned (8) to aligned (4) to see another possibility.

/* PR target/65956 */

struct B { char *a, *b; };
typedef struct B C __attribute__((aligned (8)));
struct A { C a; int b; long long c; };
char v[3];

__attribute__((noinline, noclone)) void
fn1 (int v, ...)
{
  __builtin_va_list ap;
  __builtin_va_start (ap, v);
  C c, d;
  c = __builtin_va_arg (ap, C);
  __builtin_va_arg (ap, int);
  d = __builtin_va_arg (ap, C);
  __builtin_va_end (ap);
  if (c.a != &v[1] || d.a != &v[2])
    __builtin_abort ();
  v[1]++;
}

__attribute__((noinline, noclone)) int
fn2 (C x)
{
  asm volatile ("" : "+g" (x.a) : : "memory");
  asm volatile ("" : "+g" (x.b) : : "memory");
  return x.a == &v[0];
}

__attribute__((noinline, noclone)) void
fn3 (const char *x)
{
  if (x[0] != 0)
    __builtin_abort ();
}

static struct A
foo (const char *x, struct A y, struct A z)
{
  struct A r = { { 0, 0 }, 0, 0 };
  if (y.b && z.b)
    {
      if (fn2 (y.a) && fn2 (z.a))
	switch (x[0])
	  {
	  case '|':
	    break;
	  default:
	    fn3 (x);
	  }
      fn1 (0, y.a, 0, z.a);
    }
  return r;
}

__attribute__((noinline, noclone)) int
bar (int x, struct A *y)
{
  switch (x)
    {
    case 219:
      foo ("+", y[-2], y[0]);
    case 220:
      foo ("-", y[-2], y[0]);
    }
}

int
main ()
{
  struct A a[3] = { { { &v[1], &v[0] }, 1, 1LL },
		    { { &v[0], &v[0] }, 0, 0LL },
		    { { &v[2], &v[0] }, 2, 2LL } };
  bar (220, a + 2);
  if (v[1] != 1)
    __builtin_abort ();
  return 0;
}

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
> So at least changing arm_needs_doubleword_align for non-aggregates would
> likely not break anything that hasn't been broken already and would unbreak
> the majority of cases.

Attached (untested so far).  It indeed changes code generated for
over-aligned va_arg, but as I believe you can't properly pass those in the
... caller, this should just fix it so that va_arg handling matches the
caller (and likewise for callees for named argument passing).

> The following testcase shows that eipa_sra changes alignment even for the
> aggregates.  Change aligned (8) to aligned (4) to see another possibility.

Actually I misread it, for the aggregates esra actually doesn't change
anything, which is the reason why the testcase doesn't fail.
The problem with the scalars is that esra first changed it to the
over-aligned MEM_REFs and then later on eipa_sra used the types of the
MEM_REFs created by esra.

2015-05-05  Jakub Jelinek  <jakub@redhat.com>

	PR target/65956
	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type itself.

	* gcc.c-torture/execute/pr65956.c: New test.

--- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
+++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
@@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
 static bool
 arm_needs_doubleword_align (machine_mode mode, const_tree type)
 {
-  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
-	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
+  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
+    return true;
+  if (type == NULL_TREE)
+    return false;
+  if (AGGREGATE_TYPE_P (type))
+    return TYPE_ALIGN (type) > PARM_BOUNDARY;
+  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
 }
 
 
--- gcc/testsuite/gcc.c-torture/execute/pr65956.c.jj	2015-05-01 10:32:34.730150257 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr65956.c	2015-05-01 10:32:13.000000000 +0200
@@ -0,0 +1,67 @@
+/* PR target/65956 */
+
+struct A { char *a; int b; long long c; };
+char v[3];
+
+__attribute__((noinline, noclone)) void
+fn1 (char *x, char *y)
+{
+  if (x != &v[1] || y != &v[2])
+    __builtin_abort ();
+  v[1]++;
+}
+
+__attribute__((noinline, noclone)) int
+fn2 (char *x)
+{
+  asm volatile ("" : "+g" (x) : : "memory");
+  return x == &v[0];
+}
+
+__attribute__((noinline, noclone)) void
+fn3 (const char *x)
+{
+  if (x[0] != 0)
+    __builtin_abort ();
+}
+
+static struct A
+foo (const char *x, struct A y, struct A z)
+{
+  struct A r = { 0, 0, 0 };
+  if (y.b && z.b)
+    {
+      if (fn2 (y.a) && fn2 (z.a))
+	switch (x[0])
+	  {
+	  case '|':
+	    break;
+	  default:
+	    fn3 (x);
+	  }
+      fn1 (y.a, z.a);
+    }
+  return r;
+}
+
+__attribute__((noinline, noclone)) int
+bar (int x, struct A *y)
+{
+  switch (x)
+    {
+    case 219:
+      foo ("+", y[-2], y[0]);
+    case 220:
+      foo ("-", y[-2], y[0]);
+    }
+}
+
+int
+main ()
+{
+  struct A a[3] = { { &v[1], 1, 1LL }, { &v[0], 0, 0LL }, { &v[2], 2, 2LL } };
+  bar (220, a + 2);
+  if (v[1] != 1)
+    __builtin_abort ();
+  return 0;
+}


	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 08:32, Jakub Jelinek wrote:
> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>> So at least changing arm_needs_doubleword_align for non-aggregates would
>> likely not break anything that hasn't been broken already and would unbreak
>> the majority of cases.
> 
> Attached (untested so far).  It indeed changes code generated for
> over-aligned va_arg, but as I believe you can't properly pass those in the
> ... caller, this should just fix it so that va_arg handling matches the
> caller (and likewise for callees for named argument passing).
> 
>> The following testcase shows that eipa_sra changes alignment even for the
>> aggregates.  Change aligned (8) to aligned (4) to see another possibility.
> 
> Actually I misread it, for the aggregates esra actually doesn't change
> anything, which is the reason why the testcase doesn't fail.
> The problem with the scalars is that esra first changed it to the
> over-aligned MEM_REFs and then later on eipa_sra used the types of the
> MEM_REFs created by esra.
> 
> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/65956
> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type itself.
> 
> 	* gcc.c-torture/execute/pr65956.c: New test.
> 
> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>  static bool
>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>  {
> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
> +    return true;

I don't think this is right (though I suspect the existing code has the
same problem).  We should only look at mode if there is no type
information.  The problem is that GCC has a nasty habit of assigning
real machine modes to things that are really BLKmode and we've run into
several cases where this has royally screwed things up.  So for
consistency in the ARM back-end we are careful to only use mode when
type is NULL (=> it's a libcall).

> +  if (type == NULL_TREE)
> +    return false;
> +  if (AGGREGATE_TYPE_P (type))
> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>  }
>  



It ought to be possible to re-order this, though, to

 static bool
 arm_needs_doubleword_align (machine_mode mode, const_tree type)
 {
-  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
-	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
+  if (type != NULL_TREE)
+    {
+      if (AGGREGATE_TYPE_P (type))
+        return TYPE_ALIGN (type) > PARM_BOUNDARY;
+      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
+    }
+  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
 }


Either way, this would need careful cross-testing against an existing
compiler.

R.

>  
> --- gcc/testsuite/gcc.c-torture/execute/pr65956.c.jj	2015-05-01 10:32:34.730150257 +0200
> +++ gcc/testsuite/gcc.c-torture/execute/pr65956.c	2015-05-01 10:32:13.000000000 +0200
> @@ -0,0 +1,67 @@
> +/* PR target/65956 */
> +
> +struct A { char *a; int b; long long c; };
> +char v[3];
> +
> +__attribute__((noinline, noclone)) void
> +fn1 (char *x, char *y)
> +{
> +  if (x != &v[1] || y != &v[2])
> +    __builtin_abort ();
> +  v[1]++;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +fn2 (char *x)
> +{
> +  asm volatile ("" : "+g" (x) : : "memory");
> +  return x == &v[0];
> +}
> +
> +__attribute__((noinline, noclone)) void
> +fn3 (const char *x)
> +{
> +  if (x[0] != 0)
> +    __builtin_abort ();
> +}
> +
> +static struct A
> +foo (const char *x, struct A y, struct A z)
> +{
> +  struct A r = { 0, 0, 0 };
> +  if (y.b && z.b)
> +    {
> +      if (fn2 (y.a) && fn2 (z.a))
> +	switch (x[0])
> +	  {
> +	  case '|':
> +	    break;
> +	  default:
> +	    fn3 (x);
> +	  }
> +      fn1 (y.a, z.a);
> +    }
> +  return r;
> +}
> +
> +__attribute__((noinline, noclone)) int
> +bar (int x, struct A *y)
> +{
> +  switch (x)
> +    {
> +    case 219:
> +      foo ("+", y[-2], y[0]);
> +    case 220:
> +      foo ("-", y[-2], y[0]);
> +    }
> +}
> +
> +int
> +main ()
> +{
> +  struct A a[3] = { { &v[1], 1, 1LL }, { &v[0], 0, 0LL }, { &v[2], 2, 2LL } };
> +  bar (220, a + 2);
> +  if (v[1] != 1)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
> 
> 	Jakub
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 11:54, Richard Earnshaw wrote:
> On 05/05/15 08:32, Jakub Jelinek wrote:
>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>> So at least changing arm_needs_doubleword_align for non-aggregates would
>>> likely not break anything that hasn't been broken already and would unbreak
>>> the majority of cases.
>>
>> Attached (untested so far).  It indeed changes code generated for
>> over-aligned va_arg, but as I believe you can't properly pass those in the
>> ... caller, this should just fix it so that va_arg handling matches the
>> caller (and likewise for callees for named argument passing).
>>
>>> The following testcase shows that eipa_sra changes alignment even for the
>>> aggregates.  Change aligned (8) to aligned (4) to see another possibility.
>>
>> Actually I misread it, for the aggregates esra actually doesn't change
>> anything, which is the reason why the testcase doesn't fail.
>> The problem with the scalars is that esra first changed it to the
>> over-aligned MEM_REFs and then later on eipa_sra used the types of the
>> MEM_REFs created by esra.
>>
>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>
>> 	PR target/65956
>> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type itself.
>>
>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>
>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>  static bool
>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>  {
>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>> +    return true;
> 
> I don't think this is right (though I suspect the existing code has the
> same problem).  We should only look at mode if there is no type
> information.  The problem is that GCC has a nasty habit of assigning
> real machine modes to things that are really BLKmode and we've run into
> several cases where this has royally screwed things up.  So for
> consistency in the ARM back-end we are careful to only use mode when
> type is NULL (=> it's a libcall).
> 
>> +  if (type == NULL_TREE)
>> +    return false;
>> +  if (AGGREGATE_TYPE_P (type))
>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>  }
>>  
> 
> 
> 
> It ought to be possible to re-order this, though, to
> 
>  static bool
>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>  {
> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
> +  if (type != NULL_TREE)
> +    {
> +      if (AGGREGATE_TYPE_P (type))
> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
> +    }
> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>  }
> 
> 
> Either way, this would need careful cross-testing against an existing
> compiler.
> 

It looks as though either patch would cause ABI incompatibility for

typedef int alignedint __attribute__((aligned((8))));

int  __attribute__((weak)) foo (int a, alignedint b)
{return b;}

void bar (alignedint x)
{
  foo (1, x);
}

Where currently gcc uses r2 as the argument register for b in foo.

R.

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Tue, May 05, 2015 at 12:01:59PM +0100, Richard Earnshaw wrote:
> > Either way, this would need careful cross-testing against an existing
> > compiler.
> > 
> 
> It looks as though either patch would cause ABI incompatibility for
> 
> typedef int alignedint __attribute__((aligned((8))));
> 
> int  __attribute__((weak)) foo (int a, alignedint b)
> {return b;}
> 
> void bar (alignedint x)
> {
>   foo (1, x);
> }
> 
> Where currently gcc uses r2 as the argument register for b in foo.

That is true, but as you can't call it with almost anything else:

void bar2 (alignedint x)
{
  foo (1, (alignedint) 1);
}

void bar3 (alignedint x)
{
  foo (1, x + (alignedint) 1);
}

alignedint q;

void bar4 (alignedint x)
{
  foo (1, q);
}

eextern int baz (int, int);

void bar5 (alignedint x)
{
  baz (1, x);
}

is all broken, I'd seriously doubt anybody is actually using it
successfully.
Having an ABI feature that most of the time doesn't work at
all, and occassionally happens to work, is just unsupportable.

You could add a -Wpsabi warning in the callee, warning if any of the
non-aggregate arguments has
TYPE_ALIGN (type) != TYPE_ALIGN (TYPE_MAIN_VARIANT (type))
to let users know that this really didn't work in <= 5.1.
And supposedly we need the tree-sra.c patch then so that eipa_sra
doesn't create the over-aligned parameters, because -Wpsabi then would warn
about those.

In any case, this is a P1 issue that needs resolving RSN, not just on the
trunk, but also on the branch, and the tree-sra.c patch I've posted isn't
anywhere close to resolving it without fixing the backend.

I'm in the middle of bootstrapping/regtesting this on armv7hl (both
profiledbootstrap to see if it fixes it and normal one).

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Biener]

On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>On 05/05/15 11:54, Richard Earnshaw wrote:
>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>> So at least changing arm_needs_doubleword_align for non-aggregates
>would
>>>> likely not break anything that hasn't been broken already and would
>unbreak
>>>> the majority of cases.
>>>
>>> Attached (untested so far).  It indeed changes code generated for
>>> over-aligned va_arg, but as I believe you can't properly pass those
>in the
>>> ... caller, this should just fix it so that va_arg handling matches
>the
>>> caller (and likewise for callees for named argument passing).
>>>
>>>> The following testcase shows that eipa_sra changes alignment even
>for the
>>>> aggregates.  Change aligned (8) to aligned (4) to see another
>possibility.
>>>
>>> Actually I misread it, for the aggregates esra actually doesn't
>change
>>> anything, which is the reason why the testcase doesn't fail.
>>> The problem with the scalars is that esra first changed it to the
>>> over-aligned MEM_REFs and then later on eipa_sra used the types of
>the
>>> MEM_REFs created by esra.
>>>
>>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR target/65956
>>> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
>>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>itself.
>>>
>>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>>
>>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>  static bool
>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>  {
>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>> +    return true;
>> 
>> I don't think this is right (though I suspect the existing code has
>the
>> same problem).  We should only look at mode if there is no type
>> information.  The problem is that GCC has a nasty habit of assigning
>> real machine modes to things that are really BLKmode and we've run
>into
>> several cases where this has royally screwed things up.  So for
>> consistency in the ARM back-end we are careful to only use mode when
>> type is NULL (=> it's a libcall).
>> 
>>> +  if (type == NULL_TREE)
>>> +    return false;
>>> +  if (AGGREGATE_TYPE_P (type))
>>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>  }
>>>  
>> 
>> 
>> 
>> It ought to be possible to re-order this, though, to
>> 
>>  static bool
>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>  {
>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>> +  if (type != NULL_TREE)
>> +    {
>> +      if (AGGREGATE_TYPE_P (type))
>> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
>> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>> +    }
>> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>  }
>> 
>> 
>> Either way, this would need careful cross-testing against an existing
>> compiler.
>> 
>
>It looks as though either patch would cause ABI incompatibility for
>
>typedef int alignedint __attribute__((aligned((8))));
>
>int  __attribute__((weak)) foo (int a, alignedint b)
>{return b;}
>
>void bar (alignedint x)
>{
>  foo (1, x);
>}
>
>Where currently gcc uses r2 as the argument register for b in foo.

And for foo (1,2) or an int typed 2nd arg?

Richard.

>R.

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 13:37, Richard Biener wrote:
> On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>> On 05/05/15 11:54, Richard Earnshaw wrote:
>>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>>> So at least changing arm_needs_doubleword_align for non-aggregates
>> would
>>>>> likely not break anything that hasn't been broken already and would
>> unbreak
>>>>> the majority of cases.
>>>>
>>>> Attached (untested so far).  It indeed changes code generated for
>>>> over-aligned va_arg, but as I believe you can't properly pass those
>> in the
>>>> ... caller, this should just fix it so that va_arg handling matches
>> the
>>>> caller (and likewise for callees for named argument passing).
>>>>
>>>>> The following testcase shows that eipa_sra changes alignment even
>> for the
>>>>> aggregates.  Change aligned (8) to aligned (4) to see another
>> possibility.
>>>>
>>>> Actually I misread it, for the aggregates esra actually doesn't
>> change
>>>> anything, which is the reason why the testcase doesn't fail.
>>>> The problem with the scalars is that esra first changed it to the
>>>> over-aligned MEM_REFs and then later on eipa_sra used the types of
>> the
>>>> MEM_REFs created by esra.
>>>>
>>>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>>>
>>>> 	PR target/65956
>>>> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
>>>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>> itself.
>>>>
>>>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>>>
>>>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>>>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>>  static bool
>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>  {
>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>>> +    return true;
>>>
>>> I don't think this is right (though I suspect the existing code has
>> the
>>> same problem).  We should only look at mode if there is no type
>>> information.  The problem is that GCC has a nasty habit of assigning
>>> real machine modes to things that are really BLKmode and we've run
>> into
>>> several cases where this has royally screwed things up.  So for
>>> consistency in the ARM back-end we are careful to only use mode when
>>> type is NULL (=> it's a libcall).
>>>
>>>> +  if (type == NULL_TREE)
>>>> +    return false;
>>>> +  if (AGGREGATE_TYPE_P (type))
>>>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>>  }
>>>>  
>>>
>>>
>>>
>>> It ought to be possible to re-order this, though, to
>>>
>>>  static bool
>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>  {
>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>> +  if (type != NULL_TREE)
>>> +    {
>>> +      if (AGGREGATE_TYPE_P (type))
>>> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>> +    }
>>> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>>  }
>>>
>>>
>>> Either way, this would need careful cross-testing against an existing
>>> compiler.
>>>
>>
>> It looks as though either patch would cause ABI incompatibility for
>>
>> typedef int alignedint __attribute__((aligned((8))));
>>
>> int  __attribute__((weak)) foo (int a, alignedint b)
>> {return b;}
>>
>> void bar (alignedint x)
>> {
>>  foo (1, x);
>> }
>>
>> Where currently gcc uses r2 as the argument register for b in foo.
> 
> And for foo (1,2) or an int typed 2nd arg?
> 

Yep, looks like that's horribly broken too ;-(

R.

> Richard.
> 
>> R.
> 
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 13:37, Richard Biener wrote:
> On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>> On 05/05/15 11:54, Richard Earnshaw wrote:
>>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>>> So at least changing arm_needs_doubleword_align for non-aggregates
>> would
>>>>> likely not break anything that hasn't been broken already and would
>> unbreak
>>>>> the majority of cases.
>>>>
>>>> Attached (untested so far).  It indeed changes code generated for
>>>> over-aligned va_arg, but as I believe you can't properly pass those
>> in the
>>>> ... caller, this should just fix it so that va_arg handling matches
>> the
>>>> caller (and likewise for callees for named argument passing).
>>>>
>>>>> The following testcase shows that eipa_sra changes alignment even
>> for the
>>>>> aggregates.  Change aligned (8) to aligned (4) to see another
>> possibility.
>>>>
>>>> Actually I misread it, for the aggregates esra actually doesn't
>> change
>>>> anything, which is the reason why the testcase doesn't fail.
>>>> The problem with the scalars is that esra first changed it to the
>>>> over-aligned MEM_REFs and then later on eipa_sra used the types of
>> the
>>>> MEM_REFs created by esra.
>>>>
>>>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>>>
>>>> 	PR target/65956
>>>> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
>>>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>> itself.
>>>>
>>>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>>>
>>>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>>>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>>  static bool
>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>  {
>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>>> +    return true;
>>>
>>> I don't think this is right (though I suspect the existing code has
>> the
>>> same problem).  We should only look at mode if there is no type
>>> information.  The problem is that GCC has a nasty habit of assigning
>>> real machine modes to things that are really BLKmode and we've run
>> into
>>> several cases where this has royally screwed things up.  So for
>>> consistency in the ARM back-end we are careful to only use mode when
>>> type is NULL (=> it's a libcall).
>>>
>>>> +  if (type == NULL_TREE)
>>>> +    return false;
>>>> +  if (AGGREGATE_TYPE_P (type))
>>>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>>  }
>>>>  
>>>
>>>
>>>
>>> It ought to be possible to re-order this, though, to
>>>
>>>  static bool
>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>  {
>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>> +  if (type != NULL_TREE)
>>> +    {
>>> +      if (AGGREGATE_TYPE_P (type))
>>> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>> +    }
>>> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>>  }
>>>
>>>
>>> Either way, this would need careful cross-testing against an existing
>>> compiler.
>>>
>>
>> It looks as though either patch would cause ABI incompatibility for
>>
>> typedef int alignedint __attribute__((aligned((8))));
>>
>> int  __attribute__((weak)) foo (int a, alignedint b)
>> {return b;}
>>
>> void bar (alignedint x)
>> {
>>  foo (1, x);
>> }
>>
>> Where currently gcc uses r2 as the argument register for b in foo.
> 
> And for foo (1,2) or an int typed 2nd arg?
> 
> Richard.
> 
>> R.
> 
> 


As is

int foo2 (int a, int b);

void bar2 (alignedint x)
{
  foo2 (1, x);
}

...

R

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 13:46, Richard Earnshaw wrote:
> On 05/05/15 13:37, Richard Biener wrote:
>> On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>>> On 05/05/15 11:54, Richard Earnshaw wrote:
>>>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>>>> So at least changing arm_needs_doubleword_align for non-aggregates
>>> would
>>>>>> likely not break anything that hasn't been broken already and would
>>> unbreak
>>>>>> the majority of cases.
>>>>>
>>>>> Attached (untested so far).  It indeed changes code generated for
>>>>> over-aligned va_arg, but as I believe you can't properly pass those
>>> in the
>>>>> ... caller, this should just fix it so that va_arg handling matches
>>> the
>>>>> caller (and likewise for callees for named argument passing).
>>>>>
>>>>>> The following testcase shows that eipa_sra changes alignment even
>>> for the
>>>>>> aggregates.  Change aligned (8) to aligned (4) to see another
>>> possibility.
>>>>>
>>>>> Actually I misread it, for the aggregates esra actually doesn't
>>> change
>>>>> anything, which is the reason why the testcase doesn't fail.
>>>>> The problem with the scalars is that esra first changed it to the
>>>>> over-aligned MEM_REFs and then later on eipa_sra used the types of
>>> the
>>>>> MEM_REFs created by esra.
>>>>>
>>>>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>>>>
>>>>> 	PR target/65956
>>>>> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
>>>>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>>> itself.
>>>>>
>>>>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>>>>
>>>>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>>>>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>>>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>>>  static bool
>>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>>  {
>>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>>>> +    return true;
>>>>
>>>> I don't think this is right (though I suspect the existing code has
>>> the
>>>> same problem).  We should only look at mode if there is no type
>>>> information.  The problem is that GCC has a nasty habit of assigning
>>>> real machine modes to things that are really BLKmode and we've run
>>> into
>>>> several cases where this has royally screwed things up.  So for
>>>> consistency in the ARM back-end we are careful to only use mode when
>>>> type is NULL (=> it's a libcall).
>>>>
>>>>> +  if (type == NULL_TREE)
>>>>> +    return false;
>>>>> +  if (AGGREGATE_TYPE_P (type))
>>>>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>>>  }
>>>>>  
>>>>
>>>>
>>>>
>>>> It ought to be possible to re-order this, though, to
>>>>
>>>>  static bool
>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>  {
>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>> +  if (type != NULL_TREE)
>>>> +    {
>>>> +      if (AGGREGATE_TYPE_P (type))
>>>> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>> +    }
>>>> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>>>  }
>>>>
>>>>
>>>> Either way, this would need careful cross-testing against an existing
>>>> compiler.
>>>>
>>>
>>> It looks as though either patch would cause ABI incompatibility for
>>>
>>> typedef int alignedint __attribute__((aligned((8))));
>>>
>>> int  __attribute__((weak)) foo (int a, alignedint b)
>>> {return b;}
>>>
>>> void bar (alignedint x)
>>> {
>>>  foo (1, x);
>>> }
>>>
>>> Where currently gcc uses r2 as the argument register for b in foo.
>>
>> And for foo (1,2) or an int typed 2nd arg?
>>
>> Richard.
>>
>>> R.
>>
>>
> 
> 
> As is
> 
> int foo2 (int a, int b);
> 
> void bar2 (alignedint x)
> {
>   foo2 (1, x);
> }
> 

The real question here is why is TYPE the type of the value, rather than
the type of the formal as expressed by the prototype (or implicit
prototype in the case of variadics or K&R)?  Surely this is the mid-end
passing the wrong information to the back-end.

R.

> ...
> 
> R
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Tue, May 05, 2015 at 01:49:55PM +0100, Richard Earnshaw wrote:
> The real question here is why is TYPE the type of the value, rather than
> the type of the formal as expressed by the prototype (or implicit
> prototype in the case of variadics or K&R)?  Surely this is the mid-end
> passing the wrong information to the back-end.

There is nothing else for unnamed arguments (K&R, stdarg).
For named arguments, the backend has the option to save the fntype in
CUMULATIVE_ARGS and look it up when it needs that.  But, that will
still mean K&R and stdarg will be just broken on arm.

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 13:54, Jakub Jelinek wrote:
> On Tue, May 05, 2015 at 01:49:55PM +0100, Richard Earnshaw wrote:
>> The real question here is why is TYPE the type of the value, rather than
>> the type of the formal as expressed by the prototype (or implicit
>> prototype in the case of variadics or K&R)?  Surely this is the mid-end
>> passing the wrong information to the back-end.
> 
> There is nothing else for unnamed arguments (K&R, stdarg).
> For named arguments, the backend has the option to save the fntype in
> CUMULATIVE_ARGS and look it up when it needs that.  But, that will
> still mean K&R and stdarg will be just broken on arm.
> 
> 	Jakub
> 

In a literal sense, yes.  However, even K&R & stdarg have standard
promotion and conversion rules (size < int => int, floats promoted to
double, etc).  What are those rules for GCC's overaligned types (ie
where in the docs does it say what happens and how a back-end should
interpret them)?  Shouldn't the mid-end be doing that work so as to
create a consistent view of the values passed into the back-end?  It
seems to me that at present the back-end has to be psychic as to what is
really happening.

R.

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
> In a literal sense, yes.  However, even K&R & stdarg have standard
> promotion and conversion rules (size < int => int, floats promoted to
> double, etc).  What are those rules for GCC's overaligned types (ie
> where in the docs does it say what happens and how a back-end should
> interpret them)?  Shouldn't the mid-end be doing that work so as to

For the middle-end, the TYPE_ALIGN info on expression types is considered
useless, you can get there anything.  There is no conversion rule to what
you get for myalignedint + int, or (myalignedint) int, or (int)
myalignedint, etc.

> create a consistent view of the values passed into the back-end?  It
> seems to me that at present the back-end has to be psychic as to what is
> really happening.

No, the backend just shouldn't consider TYPE_ALIGN on the scalars, and it
seems only arm ever looks at that.

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 14:06, Jakub Jelinek wrote:
> On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
>> In a literal sense, yes.  However, even K&R & stdarg have standard
>> promotion and conversion rules (size < int => int, floats promoted to
>> double, etc).  What are those rules for GCC's overaligned types (ie
>> where in the docs does it say what happens and how a back-end should
>> interpret them)?  Shouldn't the mid-end be doing that work so as to
> 
> For the middle-end, the TYPE_ALIGN info on expression types is considered
> useless, you can get there anything.  There is no conversion rule to what
> you get for myalignedint + int, or (myalignedint) int, or (int)
> myalignedint, etc.
> 
>> create a consistent view of the values passed into the back-end?  It
>> seems to me that at present the back-end has to be psychic as to what is
>> really happening.
> 
> No, the backend just shouldn't consider TYPE_ALIGN on the scalars, and it
> seems only arm ever looks at that.
> 

Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
related functions) makes any mention of this...

R.

> 	Jakub
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Biener]

On May 5, 2015 2:49:55 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>On 05/05/15 13:46, Richard Earnshaw wrote:
>> On 05/05/15 13:37, Richard Biener wrote:
>>> On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw
><Richard.Earnshaw@foss.arm.com> wrote:
>>>> On 05/05/15 11:54, Richard Earnshaw wrote:
>>>>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>>>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>>>>> So at least changing arm_needs_doubleword_align for
>non-aggregates
>>>> would
>>>>>>> likely not break anything that hasn't been broken already and
>would
>>>> unbreak
>>>>>>> the majority of cases.
>>>>>>
>>>>>> Attached (untested so far).  It indeed changes code generated for
>>>>>> over-aligned va_arg, but as I believe you can't properly pass
>those
>>>> in the
>>>>>> ... caller, this should just fix it so that va_arg handling
>matches
>>>> the
>>>>>> caller (and likewise for callees for named argument passing).
>>>>>>
>>>>>>> The following testcase shows that eipa_sra changes alignment
>even
>>>> for the
>>>>>>> aggregates.  Change aligned (8) to aligned (4) to see another
>>>> possibility.
>>>>>>
>>>>>> Actually I misread it, for the aggregates esra actually doesn't
>>>> change
>>>>>> anything, which is the reason why the testcase doesn't fail.
>>>>>> The problem with the scalars is that esra first changed it to the
>>>>>> over-aligned MEM_REFs and then later on eipa_sra used the types
>of
>>>> the
>>>>>> MEM_REFs created by esra.
>>>>>>
>>>>>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>>>>>
>>>>>> 	PR target/65956
>>>>>> 	* config/arm/arm.c (arm_needs_doubleword_align): For
>non-aggregate
>>>>>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>>>> itself.
>>>>>>
>>>>>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>>>>>
>>>>>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>>>>>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>>>>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>>>>  static bool
>>>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>>>  {
>>>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>>>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>>>>> +    return true;
>>>>>
>>>>> I don't think this is right (though I suspect the existing code
>has
>>>> the
>>>>> same problem).  We should only look at mode if there is no type
>>>>> information.  The problem is that GCC has a nasty habit of
>assigning
>>>>> real machine modes to things that are really BLKmode and we've run
>>>> into
>>>>> several cases where this has royally screwed things up.  So for
>>>>> consistency in the ARM back-end we are careful to only use mode
>when
>>>>> type is NULL (=> it's a libcall).
>>>>>
>>>>>> +  if (type == NULL_TREE)
>>>>>> +    return false;
>>>>>> +  if (AGGREGATE_TYPE_P (type))
>>>>>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>>>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>>>>  }
>>>>>>  
>>>>>
>>>>>
>>>>>
>>>>> It ought to be possible to re-order this, though, to
>>>>>
>>>>>  static bool
>>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>>  {
>>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>>> +  if (type != NULL_TREE)
>>>>> +    {
>>>>> +      if (AGGREGATE_TYPE_P (type))
>>>>> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>>> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) >
>PARM_BOUNDARY;
>>>>> +    }
>>>>> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>>>>  }
>>>>>
>>>>>
>>>>> Either way, this would need careful cross-testing against an
>existing
>>>>> compiler.
>>>>>
>>>>
>>>> It looks as though either patch would cause ABI incompatibility for
>>>>
>>>> typedef int alignedint __attribute__((aligned((8))));
>>>>
>>>> int  __attribute__((weak)) foo (int a, alignedint b)
>>>> {return b;}
>>>>
>>>> void bar (alignedint x)
>>>> {
>>>>  foo (1, x);
>>>> }
>>>>
>>>> Where currently gcc uses r2 as the argument register for b in foo.
>>>
>>> And for foo (1,2) or an int typed 2nd arg?
>>>
>>> Richard.
>>>
>>>> R.
>>>
>>>
>> 
>> 
>> As is
>> 
>> int foo2 (int a, int b);
>> 
>> void bar2 (alignedint x)
>> {
>>   foo2 (1, x);
>> }
>> 
>
>The real question here is why is TYPE the type of the value, rather
>than
>the type of the formal as expressed by the prototype (or implicit
>prototype in the case of variadics or K&R)?  Surely this is the mid-end
>passing the wrong information to the back-end.

No - the issue is you are looking at the type of the value at all, instead of at the type of the formal.

Richard.

>R.
>
>> ...
>> 
>> R
>> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 15:06, Richard Biener wrote:
> On May 5, 2015 2:49:55 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>> On 05/05/15 13:46, Richard Earnshaw wrote:
>>> On 05/05/15 13:37, Richard Biener wrote:
>>>> On May 5, 2015 1:01:59 PM GMT+02:00, Richard Earnshaw
>> <Richard.Earnshaw@foss.arm.com> wrote:
>>>>> On 05/05/15 11:54, Richard Earnshaw wrote:
>>>>>> On 05/05/15 08:32, Jakub Jelinek wrote:
>>>>>>> On Mon, May 04, 2015 at 05:00:11PM +0200, Jakub Jelinek wrote:
>>>>>>>> So at least changing arm_needs_doubleword_align for
>> non-aggregates
>>>>> would
>>>>>>>> likely not break anything that hasn't been broken already and
>> would
>>>>> unbreak
>>>>>>>> the majority of cases.
>>>>>>>
>>>>>>> Attached (untested so far).  It indeed changes code generated for
>>>>>>> over-aligned va_arg, but as I believe you can't properly pass
>> those
>>>>> in the
>>>>>>> ... caller, this should just fix it so that va_arg handling
>> matches
>>>>> the
>>>>>>> caller (and likewise for callees for named argument passing).
>>>>>>>
>>>>>>>> The following testcase shows that eipa_sra changes alignment
>> even
>>>>> for the
>>>>>>>> aggregates.  Change aligned (8) to aligned (4) to see another
>>>>> possibility.
>>>>>>>
>>>>>>> Actually I misread it, for the aggregates esra actually doesn't
>>>>> change
>>>>>>> anything, which is the reason why the testcase doesn't fail.
>>>>>>> The problem with the scalars is that esra first changed it to the
>>>>>>> over-aligned MEM_REFs and then later on eipa_sra used the types
>> of
>>>>> the
>>>>>>> MEM_REFs created by esra.
>>>>>>>
>>>>>>> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
>>>>>>>
>>>>>>> 	PR target/65956
>>>>>>> 	* config/arm/arm.c (arm_needs_doubleword_align): For
>> non-aggregate
>>>>>>> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type
>>>>> itself.
>>>>>>>
>>>>>>> 	* gcc.c-torture/execute/pr65956.c: New test.
>>>>>>>
>>>>>>> --- gcc/config/arm/arm.c.jj	2015-05-04 21:51:42.000000000 +0200
>>>>>>> +++ gcc/config/arm/arm.c	2015-05-05 09:20:52.481693337 +0200
>>>>>>> @@ -6063,8 +6063,13 @@ arm_init_cumulative_args (CUMULATIVE_ARG
>>>>>>>  static bool
>>>>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>>>>  {
>>>>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>>>>> +  if (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY)
>>>>>>> +    return true;
>>>>>>
>>>>>> I don't think this is right (though I suspect the existing code
>> has
>>>>> the
>>>>>> same problem).  We should only look at mode if there is no type
>>>>>> information.  The problem is that GCC has a nasty habit of
>> assigning
>>>>>> real machine modes to things that are really BLKmode and we've run
>>>>> into
>>>>>> several cases where this has royally screwed things up.  So for
>>>>>> consistency in the ARM back-end we are careful to only use mode
>> when
>>>>>> type is NULL (=> it's a libcall).
>>>>>>
>>>>>>> +  if (type == NULL_TREE)
>>>>>>> +    return false;
>>>>>>> +  if (AGGREGATE_TYPE_P (type))
>>>>>>> +    return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>>>>> +  return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) > PARM_BOUNDARY;
>>>>>>>  }
>>>>>>>  
>>>>>>
>>>>>>
>>>>>>
>>>>>> It ought to be possible to re-order this, though, to
>>>>>>
>>>>>>  static bool
>>>>>>  arm_needs_doubleword_align (machine_mode mode, const_tree type)
>>>>>>  {
>>>>>> -  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY
>>>>>> -	  || (type && TYPE_ALIGN (type) > PARM_BOUNDARY));
>>>>>> +  if (type != NULL_TREE)
>>>>>> +    {
>>>>>> +      if (AGGREGATE_TYPE_P (type))
>>>>>> +        return TYPE_ALIGN (type) > PARM_BOUNDARY;
>>>>>> +      return TYPE_ALIGN (TYPE_MAIN_VARIANT (type)) >
>> PARM_BOUNDARY;
>>>>>> +    }
>>>>>> +  return (GET_MODE_ALIGNMENT (mode) > PARM_BOUNDARY);
>>>>>>  }
>>>>>>
>>>>>>
>>>>>> Either way, this would need careful cross-testing against an
>> existing
>>>>>> compiler.
>>>>>>
>>>>>
>>>>> It looks as though either patch would cause ABI incompatibility for
>>>>>
>>>>> typedef int alignedint __attribute__((aligned((8))));
>>>>>
>>>>> int  __attribute__((weak)) foo (int a, alignedint b)
>>>>> {return b;}
>>>>>
>>>>> void bar (alignedint x)
>>>>> {
>>>>>  foo (1, x);
>>>>> }
>>>>>
>>>>> Where currently gcc uses r2 as the argument register for b in foo.
>>>>
>>>> And for foo (1,2) or an int typed 2nd arg?
>>>>
>>>> Richard.
>>>>
>>>>> R.
>>>>
>>>>
>>>
>>>
>>> As is
>>>
>>> int foo2 (int a, int b);
>>>
>>> void bar2 (alignedint x)
>>> {
>>>   foo2 (1, x);
>>> }
>>>
>>
>> The real question here is why is TYPE the type of the value, rather
>> than
>> the type of the formal as expressed by the prototype (or implicit
>> prototype in the case of variadics or K&R)?  Surely this is the mid-end
>> passing the wrong information to the back-end.
> 
> No - the issue is you are looking at the type of the value at all, instead of at the type of the formal.
> 

And I would argue that passing the type of the acutal is stupid in all
cases except when the type of the formal does not exist.  It's just
asking for problems like these.

R.

> Richard.
> 
>> R.
>>
>>> ...
>>>
>>> R
>>>
> 
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Tue, May 05, 2015 at 09:32:28AM +0200, Jakub Jelinek wrote:
> 2015-05-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR target/65956
> 	* config/arm/arm.c (arm_needs_doubleword_align): For non-aggregate
> 	types check TYPE_ALIGN of TYPE_MAIN_VARIANT rather than type itself.
> 
> 	* gcc.c-torture/execute/pr65956.c: New test.

Note, the patch fixed the profiledbootstrap that has been broken in 5+.

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Tue, May 05, 2015 at 02:20:43PM +0100, Richard Earnshaw wrote:
> On 05/05/15 14:06, Jakub Jelinek wrote:
> > On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
> >> In a literal sense, yes.  However, even K&R & stdarg have standard
> >> promotion and conversion rules (size < int => int, floats promoted to
> >> double, etc).  What are those rules for GCC's overaligned types (ie
> >> where in the docs does it say what happens and how a back-end should
> >> interpret them)?  Shouldn't the mid-end be doing that work so as to
> > 
> > For the middle-end, the TYPE_ALIGN info on expression types is considered
> > useless, you can get there anything.  There is no conversion rule to what
> > you get for myalignedint + int, or (myalignedint) int, or (int)
> > myalignedint, etc.
> > 
> >> create a consistent view of the values passed into the back-end?  It
> >> seems to me that at present the back-end has to be psychic as to what is
> >> really happening.
> > 
> > No, the backend just shouldn't consider TYPE_ALIGN on the scalars, and it
> > seems only arm ever looks at that.
> > 
> 
> Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
> related functions) makes any mention of this...

While this requirement isn't documented, it is clearly common sense or at
least something any kind of testing would reveal immediately.
And it is nothing broken recently (except that with the SRA changes it hits
much more often), looking e.g. at GCC 3.2, I'm seeing that expand_call is on
that testcase also called with pretty random TYPE_ALIGN on the argument
types; we didn't have GIMPLE then, so it is nothing that GIMPLE brought in.

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 15:29, Jakub Jelinek wrote:
> On Tue, May 05, 2015 at 02:20:43PM +0100, Richard Earnshaw wrote:
>> On 05/05/15 14:06, Jakub Jelinek wrote:
>>> On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
>>>> In a literal sense, yes.  However, even K&R & stdarg have standard
>>>> promotion and conversion rules (size < int => int, floats promoted to
>>>> double, etc).  What are those rules for GCC's overaligned types (ie
>>>> where in the docs does it say what happens and how a back-end should
>>>> interpret them)?  Shouldn't the mid-end be doing that work so as to
>>>
>>> For the middle-end, the TYPE_ALIGN info on expression types is considered
>>> useless, you can get there anything.  There is no conversion rule to what
>>> you get for myalignedint + int, or (myalignedint) int, or (int)
>>> myalignedint, etc.
>>>
>>>> create a consistent view of the values passed into the back-end?  It
>>>> seems to me that at present the back-end has to be psychic as to what is
>>>> really happening.
>>>
>>> No, the backend just shouldn't consider TYPE_ALIGN on the scalars, and it
>>> seems only arm ever looks at that.
>>>
>>
>> Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
>> related functions) makes any mention of this...
> 
> While this requirement isn't documented, it is clearly common sense or at
> least something any kind of testing would reveal immediately.

Then clearly no such tests exist in the testsuite :-(

R.

> And it is nothing broken recently (except that with the SRA changes it hits
> much more often), looking e.g. at GCC 3.2, I'm seeing that expand_call is on
> that testcase also called with pretty random TYPE_ALIGN on the argument
> types; we didn't have GIMPLE then, so it is nothing that GIMPLE brought in.
> 
> 	Jakub
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 15:33, Richard Earnshaw wrote:
> On 05/05/15 15:29, Jakub Jelinek wrote:
>> On Tue, May 05, 2015 at 02:20:43PM +0100, Richard Earnshaw wrote:
>>> On 05/05/15 14:06, Jakub Jelinek wrote:
>>>> On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
>>>>> In a literal sense, yes.  However, even K&R & stdarg have standard
>>>>> promotion and conversion rules (size < int => int, floats promoted to
>>>>> double, etc).  What are those rules for GCC's overaligned types (ie
>>>>> where in the docs does it say what happens and how a back-end should
>>>>> interpret them)?  Shouldn't the mid-end be doing that work so as to
>>>>
>>>> For the middle-end, the TYPE_ALIGN info on expression types is considered
>>>> useless, you can get there anything.  There is no conversion rule to what
>>>> you get for myalignedint + int, or (myalignedint) int, or (int)
>>>> myalignedint, etc.
>>>>
>>>>> create a consistent view of the values passed into the back-end?  It
>>>>> seems to me that at present the back-end has to be psychic as to what is
>>>>> really happening.
>>>>
>>>> No, the backend just shouldn't consider TYPE_ALIGN on the scalars, and it
>>>> seems only arm ever looks at that.
>>>>
>>>
>>> Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
>>> related functions) makes any mention of this...
>>
>> While this requirement isn't documented, it is clearly common sense or at
>> least something any kind of testing would reveal immediately.
> 
> Then clearly no such tests exist in the testsuite :-(
> 

Or, more precisely, no such tests existed in 2008, when the code went in.

R.

> R.
> 
>> And it is nothing broken recently (except that with the SRA changes it hits
>> much more often), looking e.g. at GCC 3.2, I'm seeing that expand_call is on
>> that testcase also called with pretty random TYPE_ALIGN on the argument
>> types; we didn't have GIMPLE then, so it is nothing that GIMPLE brought in.
>>
>> 	Jakub
>>
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Biener]

On May 5, 2015 4:33:58 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>On 05/05/15 15:33, Richard Earnshaw wrote:
>> On 05/05/15 15:29, Jakub Jelinek wrote:
>>> On Tue, May 05, 2015 at 02:20:43PM +0100, Richard Earnshaw wrote:
>>>> On 05/05/15 14:06, Jakub Jelinek wrote:
>>>>> On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
>>>>>> In a literal sense, yes.  However, even K&R & stdarg have
>standard
>>>>>> promotion and conversion rules (size < int => int, floats
>promoted to
>>>>>> double, etc).  What are those rules for GCC's overaligned types
>(ie
>>>>>> where in the docs does it say what happens and how a back-end
>should
>>>>>> interpret them)?  Shouldn't the mid-end be doing that work so as
>to
>>>>>
>>>>> For the middle-end, the TYPE_ALIGN info on expression types is
>considered
>>>>> useless, you can get there anything.  There is no conversion rule
>to what
>>>>> you get for myalignedint + int, or (myalignedint) int, or (int)
>>>>> myalignedint, etc.
>>>>>
>>>>>> create a consistent view of the values passed into the back-end? 
>It
>>>>>> seems to me that at present the back-end has to be psychic as to
>what is
>>>>>> really happening.
>>>>>
>>>>> No, the backend just shouldn't consider TYPE_ALIGN on the scalars,
>and it
>>>>> seems only arm ever looks at that.
>>>>>
>>>>
>>>> Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
>>>> related functions) makes any mention of this...
>>>
>>> While this requirement isn't documented, it is clearly common sense
>or at
>>> least something any kind of testing would reveal immediately.
>> 
>> Then clearly no such tests exist in the testsuite :-(
>> 
>
>Or, more precisely, no such tests existed in 2008, when the code went
>in.

That just means that the code was the first that make this matter.

BTW, what do other compilers do (I suppose llvm supports the gcc extensions for alignment)?  Can llvm and gcc interoperate properly here? Do the cited testcases work with llvm?

Richard.

>
>R.
>
>> R.
>> 
>>> And it is nothing broken recently (except that with the SRA changes
>it hits
>>> much more often), looking e.g. at GCC 3.2, I'm seeing that
>expand_call is on
>>> that testcase also called with pretty random TYPE_ALIGN on the
>argument
>>> types; we didn't have GIMPLE then, so it is nothing that GIMPLE
>brought in.
>>>
>>> 	Jakub
>>>
>> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 05/05/15 19:07, Richard Biener wrote:
> On May 5, 2015 4:33:58 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>> On 05/05/15 15:33, Richard Earnshaw wrote:
>>> On 05/05/15 15:29, Jakub Jelinek wrote:
>>>> On Tue, May 05, 2015 at 02:20:43PM +0100, Richard Earnshaw wrote:
>>>>> On 05/05/15 14:06, Jakub Jelinek wrote:
>>>>>> On Tue, May 05, 2015 at 02:02:19PM +0100, Richard Earnshaw wrote:
>>>>>>> In a literal sense, yes.  However, even K&R & stdarg have
>> standard
>>>>>>> promotion and conversion rules (size < int => int, floats
>> promoted to
>>>>>>> double, etc).  What are those rules for GCC's overaligned types
>> (ie
>>>>>>> where in the docs does it say what happens and how a back-end
>> should
>>>>>>> interpret them)?  Shouldn't the mid-end be doing that work so as
>> to
>>>>>>
>>>>>> For the middle-end, the TYPE_ALIGN info on expression types is
>> considered
>>>>>> useless, you can get there anything.  There is no conversion rule
>> to what
>>>>>> you get for myalignedint + int, or (myalignedint) int, or (int)
>>>>>> myalignedint, etc.
>>>>>>
>>>>>>> create a consistent view of the values passed into the back-end? 
>> It
>>>>>>> seems to me that at present the back-end has to be psychic as to
>> what is
>>>>>>> really happening.
>>>>>>
>>>>>> No, the backend just shouldn't consider TYPE_ALIGN on the scalars,
>> and it
>>>>>> seems only arm ever looks at that.
>>>>>>
>>>>>
>>>>> Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
>>>>> related functions) makes any mention of this...
>>>>
>>>> While this requirement isn't documented, it is clearly common sense
>> or at
>>>> least something any kind of testing would reveal immediately.
>>>
>>> Then clearly no such tests exist in the testsuite :-(
>>>
>>
>> Or, more precisely, no such tests existed in 2008, when the code went
>> in.
> 
> That just means that the code was the first that make this matter.
> 

Exactly.  The comment was in response to the suggestion the code wasn't
tested.

> BTW, what do other compilers do (I suppose llvm supports the gcc extensions for alignment)?  Can llvm and gcc interoperate properly here? Do the cited testcases work with llvm?
> 

We're looking into it.  I'm talking to our LLVM team and we'll make sure
we have a consistent set of rules.

R.

> Richard.
> 
>>
>> R.
>>
>>> R.
>>>
>>>> And it is nothing broken recently (except that with the SRA changes
>> it hits
>>>> much more often), looking e.g. at GCC 3.2, I'm seeing that
>> expand_call is on
>>>> that testcase also called with pretty random TYPE_ALIGN on the
>> argument
>>>> types; we didn't have GIMPLE then, so it is nothing that GIMPLE
>> brought in.
>>>>
>>>> 	Jakub
>>>>
>>>
> 
> 

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Alan Lawrence]

Richard Biener wrote:
> On May 5, 2015 4:33:58 PM GMT+02:00, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote:
>> On 05/05/15 15:33, Richard Earnshaw wrote:
>>> On 05/05/15 15:29, Jakub Jelinek wrote:
>>>> On Tue, May 05, 2015 at 02:20:43PM +0100, Richard Earnshaw wrote:
>>>>> On 05/05/15 14:06, Jakub Jelinek wrote:
>>>>>> For the middle-end, the TYPE_ALIGN info on expression types is
>> considered
>>>>>> useless, you can get there anything.  There is no conversion rule
>> to what
>>>>>> you get for myalignedint + int, or (myalignedint) int, or (int)
>>>>>> myalignedint, etc.
>>>>>>
>>>>>>> create a consistent view of the values passed into the back-end? 
>> It
>>>>>>> seems to me that at present the back-end has to be psychic as to
>> what is
>>>>>>> really happening.
>>>>>> No, the backend just shouldn't consider TYPE_ALIGN on the scalars,
>> and it
>>>>>> seems only arm ever looks at that.
>>>>>>
>>>>> Nothing in the specification for TARGET_FUNCTION_ARG (or any of the
>>>>> related functions) makes any mention of this...
>>>> While this requirement isn't documented, it is clearly common sense
>> or at
>>>> least something any kind of testing would reveal immediately.
>>> Then clearly no such tests exist in the testsuite :-(
>>>
>> Or, more precisely, no such tests existed in 2008, when the code went
>> in.
> 
> That just means that the code was the first that make this matter.
> 
> BTW, what do other compilers do (I suppose llvm supports the gcc extensions for alignment)?  Can llvm and gcc interoperate properly here? Do the cited testcases work with llvm?
> 
> Richard.

So for my two cents, or perhaps three:

(1) It'd be great to have something in the documentation for TARGET_FUNCTION_ARG 
that explains what the contract for the type information provided is. 
Even/especially if some of this is "considered useless" i.e. provided on a best 
effort basis by compiler analysis. (What happens in other cases where there are 
subtyping-like relationships? E.g. genuine C++ subtyping, or other attributes 
like const, volatile? I don't imagine that any backend's ABI depends on those 
attributes, but do we want to rule that out, e.g. for attributes that don't 
exist yet?)

(2) If all backends were expected to save the function prototype in 
TARGET_INIT_CUMULATIVE_ARGS and use that, it'd be a shame to duplicate that. 
However, it seems this is because other backends don't depend on TYPE_ALIGN. It 
doesn't seem unreasonable for argument passing rules to depend on alignment, 
however.

(3) C++11 rules out using __align_as on function arguments, including via a 
typedef. GCC allows __attribute__ aligned on arguments *only via a typedef*. Is 
that how we want it?

--Alan

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Thu, May 07, 2015 at 12:16:32PM +0100, Alan Lawrence wrote:
> So for my two cents, or perhaps three:
> 
> (1) It'd be great to have something in the documentation for
> TARGET_FUNCTION_ARG that explains what the contract for the type information
> provided is. Even/especially if some of this is "considered useless" i.e.
> provided on a best effort basis by compiler analysis. (What happens in other
> cases where there are subtyping-like relationships? E.g. genuine C++
> subtyping, or other attributes like const, volatile? I don't imagine that
> any backend's ABI depends on those attributes, but do we want to rule that
> out, e.g. for attributes that don't exist yet?)

Sure, documentation improvements are always welcome.
Yes, const/volatile/__restrict, or various attributes like deprecated and
various others shouldn't be taken into account when deciding on argument (or
return value) passing.

> (2) If all backends were expected to save the function prototype in
> TARGET_INIT_CUMULATIVE_ARGS and use that, it'd be a shame to duplicate that.
> However, it seems this is because other backends don't depend on TYPE_ALIGN.
> It doesn't seem unreasonable for argument passing rules to depend on
> alignment, however.

Most of them don't need that.  The type they see in the callee is compatible
with the type they see in the caller (except for when the user is supposed
to ensure that, like K&R calls or va_arg), otherwise supposedly the FEs
should loudly complain.
But, stuff like alignment or various other attributes on scalar types
are considered useless for function calls by the FEs, and so the backends
should treat them IMNSHO too.  Thus argument passing rules depending on
alignment of scalars is unreasonable.

BTW, I've checked llvm and it ignores the alignment attribute on scalar
types for argument passing, so the patch I've posted, in addition to
allowing GCC to be compatible with itself, would make it compatible with
clang too in that regard.

> (3) C++11 rules out using __align_as on function arguments, including via a
> typedef. GCC allows __attribute__ aligned on arguments *only via a typedef*.
> Is that how we want it?

Trying to change a GCC extension accepted for many years is not a good idea.

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Jakub Jelinek]

On Thu, May 07, 2015 at 12:16:32PM +0100, Alan Lawrence wrote:
> So for my two cents, or perhaps three:

Any progress on this PR?
A P1 bug that affects several packages stalled for a month isn't a very good
thing... (not to mention broken profiledbootstrap on ARM due to the same
issue).
I've checked and llvm on ARM ignores the alignment on the scalar
arguments...

	Jakub

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Richard Earnshaw]

On 01/06/15 13:07, Jakub Jelinek wrote:
> On Thu, May 07, 2015 at 12:16:32PM +0100, Alan Lawrence wrote:
>> So for my two cents, or perhaps three:
> 
> Any progress on this PR?
> A P1 bug that affects several packages stalled for a month isn't a very good
> thing... (not to mention broken profiledbootstrap on ARM due to the same
> issue).
> I've checked and llvm on ARM ignores the alignment on the scalar
> arguments...
> 
> 	Jakub
> 

We're working on some updates to the ABI documents.  If we're going to
break ABI compatibility, even in some corner cases, it would make sense
to only do this once.

We need to think about more than just LLVM and GCC, so it's not as
simple as just copying what LLVM does.

Note that there's almost certainly a similar problem for AArch64, though
it is probably less common for it to manifest itself -- probably only
when structs contain 128-bit aligned objects.

R.

Re: [PATCH] Fix eipa_sra AAPCS issue (PR target/65956)

[Alan Lawrence]

Richard Earnshaw wrote:
> On 01/06/15 13:07, Jakub Jelinek wrote:
>> On Thu, May 07, 2015 at 12:16:32PM +0100, Alan Lawrence wrote:
>>> So for my two cents, or perhaps three:
>> Any progress on this PR?
>> A P1 bug that affects several packages stalled for a month isn't a very good
>> thing... (not to mention broken profiledbootstrap on ARM due to the same
>> issue).
>> I've checked and llvm on ARM ignores the alignment on the scalar
>> arguments...
>>
>> 	Jakub
>>
> 
> We're working on some updates to the ABI documents.  If we're going to
> break ABI compatibility, even in some corner cases, it would make sense
> to only do this once.

One question is whether to treat structs differently from scalars in the ABI 
specification. Structs raise lots of corner cases! I notice the following 
oddity, and wonder if anyone can shed any light on this:

typedef __attribute__((aligned(8))) struct { int x; int y; } foo;
typedef struct __attribute__((aligned(8))) { int x; int y; } bar;
typedef struct { int x; int y; } __attribute__((aligned(8))) baz;
typedef struct { int x; int y; } qux __attribute__((aligned(8)));

create typedefs (foo, bar, baz, qux) all with alignment 8, as expected. However, 
the TYPE_MAIN_VARIANT (an anonymous struct type) has alignment 4 for foo and qux 
(so the attribute has been applied only to the typedef), but alignment 8 for bar 
and baz (i.e. the attribute has been applied to the underlying struct).

--Alan