Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On Tue, Jun 07, 2016 at 12:07:03PM +0100, James Greenhalgh wrote: > On Fri, Jan 22, 2016 at 05:16:00PM +, Alan Lawrence wrote: > > > > On 21/01/16 17:23, Alan Lawrence wrote: > > > On 18/01/16 17:10, Eric Botcazou wrote: > > >> > > >> Could you post the list of files that differ? How do they differ > > >> exactly? > > > > > > Hmmm. Well, I definitely had this failing to bootstrap once. I repeated > > > that, to > > > try to identify exactly what the differences wereand it succeeded > > > even with > > > my pre-AAPCS64-update host compiler. So, this is probably a false alarm; > > > I'm > > > bootstrapping again, after a rebase, to make sure... > > > > > > --Alan > > > > Ok, rebased onto a more recent build, and bootstrapping with Ada posed no > > problems. Sorry for the noise. > > > > However, I had to drop the assert that TYPE_FIELDS was non-null because of > > some > > C++ testcases. > > > > Is this version OK for trunk? > > Now that we're in GCC7, this version of the patch is OK for trunk. > > From my reading of Richard's AAPCS update, this patch implements the > rules as required. > > I'll give this a day for any last minute comments from Richard/Marcus, > then commit this on your behalf tomorrow. I've now committed this on Alan's behalf as revisions r237224 (this patch) and r237225 (the tests) respectively. Thanks, James
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On Fri, Jan 22, 2016 at 05:16:00PM +, Alan Lawrence wrote:
>
> On 21/01/16 17:23, Alan Lawrence wrote:
> > On 18/01/16 17:10, Eric Botcazou wrote:
> >>
> >> Could you post the list of files that differ? How do they differ exactly?
> >
> > Hmmm. Well, I definitely had this failing to bootstrap once. I repeated
> > that, to
> > try to identify exactly what the differences wereand it succeeded even
> > with
> > my pre-AAPCS64-update host compiler. So, this is probably a false alarm; I'm
> > bootstrapping again, after a rebase, to make sure...
> >
> > --Alan
>
> Ok, rebased onto a more recent build, and bootstrapping with Ada posed no
> problems. Sorry for the noise.
>
> However, I had to drop the assert that TYPE_FIELDS was non-null because of
> some
> C++ testcases.
>
> Is this version OK for trunk?
Now that we're in GCC7, this version of the patch is OK for trunk.
From my reading of Richard's AAPCS update, this patch implements the
rules as required.
I'll give this a day for any last minute comments from Richard/Marcus,
then commit this on your behalf tomorrow.
Thanks,
James
> gcc/ChangeLog:
>
> * gcc/config/aarch64/aarch64.c (aarch64_function_arg_alignment):
> Rewrite, looking one level down for records and arrays.
> ---
> gcc/config/aarch64/aarch64.c | 31 ---
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 9142ac0..b084f83 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -1925,22 +1925,23 @@ aarch64_vfp_is_call_candidate (cumulative_args_t
> pcum_v, machine_mode mode,
> static unsigned int
> aarch64_function_arg_alignment (machine_mode mode, const_tree type)
> {
> - unsigned int alignment;
> + if (!type)
> +return GET_MODE_ALIGNMENT (mode);
> + if (integer_zerop (TYPE_SIZE (type)))
> +return 0;
>
> - if (type)
> -{
> - if (!integer_zerop (TYPE_SIZE (type)))
> - {
> - if (TYPE_MODE (type) == mode)
> - alignment = TYPE_ALIGN (type);
> - else
> - alignment = GET_MODE_ALIGNMENT (mode);
> - }
> - else
> - alignment = 0;
> -}
> - else
> -alignment = GET_MODE_ALIGNMENT (mode);
> + gcc_assert (TYPE_MODE (type) == mode);
> +
> + if (!AGGREGATE_TYPE_P (type))
> +return TYPE_ALIGN (TYPE_MAIN_VARIANT (type));
> +
> + if (TREE_CODE (type) == ARRAY_TYPE)
> +return TYPE_ALIGN (TREE_TYPE (type));
> +
> + unsigned int alignment = 0;
> +
> + for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> +alignment = std::max (alignment, DECL_ALIGN (field));
>
>return alignment;
> }
> --
> 1.9.1
>
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On 04/03/16 17:24, Alan Lawrence wrote:
On 26/02/16 14:52, James Greenhalgh wrote:
gcc/ChangeLog:
* gcc/config/aarch64/aarch64.c (aarch64_function_arg_alignment):
Rewrite, looking one level down for records and arrays.
---
gcc/config/aarch64/aarch64.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 9142ac0..b084f83 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1925,22 +1925,23 @@ aarch64_vfp_is_call_candidate (cumulative_args_t
pcum_v, machine_mode mode,
static unsigned int
aarch64_function_arg_alignment (machine_mode mode, const_tree type)
{
- unsigned int alignment;
+ if (!type)
+return GET_MODE_ALIGNMENT (mode);
+ if (integer_zerop (TYPE_SIZE (type)))
+return 0;
- if (type)
-{
- if (!integer_zerop (TYPE_SIZE (type)))
-{
- if (TYPE_MODE (type) == mode)
-alignment = TYPE_ALIGN (type);
- else
-alignment = GET_MODE_ALIGNMENT (mode);
-}
- else
-alignment = 0;
-}
- else
-alignment = GET_MODE_ALIGNMENT (mode);
+ gcc_assert (TYPE_MODE (type) == mode);
+
+ if (!AGGREGATE_TYPE_P (type))
+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type));
+
+ if (TREE_CODE (type) == ARRAY_TYPE)
+return TYPE_ALIGN (TREE_TYPE (type));
+
+ unsigned int alignment = 0;
+
+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+alignment = std::max (alignment, DECL_ALIGN (field));
return alignment;
}
Ping.
[snip]
I'm not proposing to backport these AArch64 changes, hence:
Ping^2.
(For gcc 7 ?)
Also tests https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01073.html .
Ping^3.
Cheers, Alan
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On 26/02/16 14:52, James Greenhalgh wrote:
gcc/ChangeLog:
* gcc/config/aarch64/aarch64.c (aarch64_function_arg_alignment):
Rewrite, looking one level down for records and arrays.
---
gcc/config/aarch64/aarch64.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 9142ac0..b084f83 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1925,22 +1925,23 @@ aarch64_vfp_is_call_candidate (cumulative_args_t
pcum_v, machine_mode mode,
static unsigned int
aarch64_function_arg_alignment (machine_mode mode, const_tree type)
{
- unsigned int alignment;
+ if (!type)
+return GET_MODE_ALIGNMENT (mode);
+ if (integer_zerop (TYPE_SIZE (type)))
+return 0;
- if (type)
-{
- if (!integer_zerop (TYPE_SIZE (type)))
- {
- if (TYPE_MODE (type) == mode)
- alignment = TYPE_ALIGN (type);
- else
- alignment = GET_MODE_ALIGNMENT (mode);
- }
- else
- alignment = 0;
-}
- else
-alignment = GET_MODE_ALIGNMENT (mode);
+ gcc_assert (TYPE_MODE (type) == mode);
+
+ if (!AGGREGATE_TYPE_P (type))
+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type));
+
+ if (TREE_CODE (type) == ARRAY_TYPE)
+return TYPE_ALIGN (TREE_TYPE (type));
+
+ unsigned int alignment = 0;
+
+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+alignment = std::max (alignment, DECL_ALIGN (field));
return alignment;
}
Ping.
If this is not appropriate for GCC6, then is it OK for Stage 1 of GCC 7?
I think this needs to be a GCC 7 fix at this point.
Additionally, I'd like to fully understand PR69841 before we take this
patch.
In particular, under what circumstances can the C++ front end set DECL_ALIGN
of a type to be wider than we expect. Can we ever end up with 128-bit
alignment of a template parameter when we were expecting 32- or 64-bit
alignment, and if we can what are the implications on this patch?
OK, so IIUC, we *should* be able to rely on DECL_ALIGN for the AAPCS64, as
PR/69841 occurred on gcc-5-branch because a C++ frontend change had not been
backported.
I'm not proposing to backport these AArch64 changes, hence:
Ping^2.
(For gcc 7 ?)
Also tests https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01073.html .
Thanks,
Alan
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On Mon, Feb 22, 2016 at 03:07:09PM +, Alan Lawrence wrote:
> On 22/01/16 17:16, Alan Lawrence wrote:
> >
> >On 21/01/16 17:23, Alan Lawrence wrote:
> >>On 18/01/16 17:10, Eric Botcazou wrote:
> >>>
> >>>Could you post the list of files that differ? How do they differ exactly?
> >>
> >>Hmmm. Well, I definitely had this failing to bootstrap once. I repeated
> >>that, to
> >>try to identify exactly what the differences wereand it succeeded even
> >>with
> >>my pre-AAPCS64-update host compiler. So, this is probably a false alarm; I'm
> >>bootstrapping again, after a rebase, to make sure...
> >>
> >>--Alan
> >
> >Ok, rebased onto a more recent build, and bootstrapping with Ada posed no
> >problems. Sorry for the noise.
> >
> >However, I had to drop the assert that TYPE_FIELDS was non-null because of
> >some
> >C++ testcases.
> >
> >Is this version OK for trunk?
> >
> >--Alan
> >
> >gcc/ChangeLog:
> >
> > * gcc/config/aarch64/aarch64.c (aarch64_function_arg_alignment):
> > Rewrite, looking one level down for records and arrays.
> >---
> > gcc/config/aarch64/aarch64.c | 31 ---
> > 1 file changed, 16 insertions(+), 15 deletions(-)
> >
> >diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >index 9142ac0..b084f83 100644
> >--- a/gcc/config/aarch64/aarch64.c
> >+++ b/gcc/config/aarch64/aarch64.c
> >@@ -1925,22 +1925,23 @@ aarch64_vfp_is_call_candidate (cumulative_args_t
> >pcum_v, machine_mode mode,
> > static unsigned int
> > aarch64_function_arg_alignment (machine_mode mode, const_tree type)
> > {
> >- unsigned int alignment;
> >+ if (!type)
> >+return GET_MODE_ALIGNMENT (mode);
> >+ if (integer_zerop (TYPE_SIZE (type)))
> >+return 0;
> >
> >- if (type)
> >-{
> >- if (!integer_zerop (TYPE_SIZE (type)))
> >-{
> >- if (TYPE_MODE (type) == mode)
> >-alignment = TYPE_ALIGN (type);
> >- else
> >-alignment = GET_MODE_ALIGNMENT (mode);
> >-}
> >- else
> >-alignment = 0;
> >-}
> >- else
> >-alignment = GET_MODE_ALIGNMENT (mode);
> >+ gcc_assert (TYPE_MODE (type) == mode);
> >+
> >+ if (!AGGREGATE_TYPE_P (type))
> >+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type));
> >+
> >+ if (TREE_CODE (type) == ARRAY_TYPE)
> >+return TYPE_ALIGN (TREE_TYPE (type));
> >+
> >+ unsigned int alignment = 0;
> >+
> >+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> >+alignment = std::max (alignment, DECL_ALIGN (field));
> >
> >return alignment;
> > }
> >
>
>
> Ping.
>
> If this is not appropriate for GCC6, then is it OK for Stage 1 of GCC 7?
I think this needs to be a GCC 7 fix at this point.
Additionally, I'd like to fully understand PR69841 before we take this
patch.
In particular, under what circumstances can the C++ front end set DECL_ALIGN
of a type to be wider than we expect. Can we ever end up with 128-bit
alignment of a template parameter when we were expecting 32- or 64-bit
alignment, and if we can what are the implications on this patch?
Thanks,
James
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On 22/01/16 17:16, Alan Lawrence wrote:
On 21/01/16 17:23, Alan Lawrence wrote:
On 18/01/16 17:10, Eric Botcazou wrote:
Could you post the list of files that differ? How do they differ exactly?
Hmmm. Well, I definitely had this failing to bootstrap once. I repeated that, to
try to identify exactly what the differences wereand it succeeded even with
my pre-AAPCS64-update host compiler. So, this is probably a false alarm; I'm
bootstrapping again, after a rebase, to make sure...
--Alan
Ok, rebased onto a more recent build, and bootstrapping with Ada posed no
problems. Sorry for the noise.
However, I had to drop the assert that TYPE_FIELDS was non-null because of some
C++ testcases.
Is this version OK for trunk?
--Alan
gcc/ChangeLog:
* gcc/config/aarch64/aarch64.c (aarch64_function_arg_alignment):
Rewrite, looking one level down for records and arrays.
---
gcc/config/aarch64/aarch64.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 9142ac0..b084f83 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1925,22 +1925,23 @@ aarch64_vfp_is_call_candidate (cumulative_args_t
pcum_v, machine_mode mode,
static unsigned int
aarch64_function_arg_alignment (machine_mode mode, const_tree type)
{
- unsigned int alignment;
+ if (!type)
+return GET_MODE_ALIGNMENT (mode);
+ if (integer_zerop (TYPE_SIZE (type)))
+return 0;
- if (type)
-{
- if (!integer_zerop (TYPE_SIZE (type)))
- {
- if (TYPE_MODE (type) == mode)
- alignment = TYPE_ALIGN (type);
- else
- alignment = GET_MODE_ALIGNMENT (mode);
- }
- else
- alignment = 0;
-}
- else
-alignment = GET_MODE_ALIGNMENT (mode);
+ gcc_assert (TYPE_MODE (type) == mode);
+
+ if (!AGGREGATE_TYPE_P (type))
+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type));
+
+ if (TREE_CODE (type) == ARRAY_TYPE)
+return TYPE_ALIGN (TREE_TYPE (type));
+
+ unsigned int alignment = 0;
+
+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+alignment = std::max (alignment, DECL_ALIGN (field));
return alignment;
}
Ping.
If this is not appropriate for GCC6, then is it OK for Stage 1 of GCC 7?
Thanks, Alan
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
> Ok, rebased onto a more recent build, and bootstrapping with Ada posed no > problems. Sorry for the noise. Great, no problem, and thanks for double checking. -- Eric Botcazou
Re: [PATCH 1/2][AArch64] Implement AAPCS64 updates for alignment attribute
On 21/01/16 17:23, Alan Lawrence wrote:
> On 18/01/16 17:10, Eric Botcazou wrote:
>>
>> Could you post the list of files that differ? How do they differ exactly?
>
> Hmmm. Well, I definitely had this failing to bootstrap once. I repeated that,
> to
> try to identify exactly what the differences wereand it succeeded even
> with
> my pre-AAPCS64-update host compiler. So, this is probably a false alarm; I'm
> bootstrapping again, after a rebase, to make sure...
>
> --Alan
Ok, rebased onto a more recent build, and bootstrapping with Ada posed no
problems. Sorry for the noise.
However, I had to drop the assert that TYPE_FIELDS was non-null because of some
C++ testcases.
Is this version OK for trunk?
--Alan
gcc/ChangeLog:
* gcc/config/aarch64/aarch64.c (aarch64_function_arg_alignment):
Rewrite, looking one level down for records and arrays.
---
gcc/config/aarch64/aarch64.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 9142ac0..b084f83 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1925,22 +1925,23 @@ aarch64_vfp_is_call_candidate (cumulative_args_t
pcum_v, machine_mode mode,
static unsigned int
aarch64_function_arg_alignment (machine_mode mode, const_tree type)
{
- unsigned int alignment;
+ if (!type)
+return GET_MODE_ALIGNMENT (mode);
+ if (integer_zerop (TYPE_SIZE (type)))
+return 0;
- if (type)
-{
- if (!integer_zerop (TYPE_SIZE (type)))
- {
- if (TYPE_MODE (type) == mode)
- alignment = TYPE_ALIGN (type);
- else
- alignment = GET_MODE_ALIGNMENT (mode);
- }
- else
- alignment = 0;
-}
- else
-alignment = GET_MODE_ALIGNMENT (mode);
+ gcc_assert (TYPE_MODE (type) == mode);
+
+ if (!AGGREGATE_TYPE_P (type))
+return TYPE_ALIGN (TYPE_MAIN_VARIANT (type));
+
+ if (TREE_CODE (type) == ARRAY_TYPE)
+return TYPE_ALIGN (TREE_TYPE (type));
+
+ unsigned int alignment = 0;
+
+ for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+alignment = std::max (alignment, DECL_ALIGN (field));
return alignment;
}
--
1.9.1
