Re: [PATCH] target/58065 ARM MALLOC_ABI_ALIGNMENT is wrong

[Ramana Radhakrishnan <ramrad01 at arm dot com>]

On 08/07/13 08:10, Bernd Edlinger wrote:
> Hello,
>
> in the discussion about the PR middle-end/57748 it became obvious that the ARM target architecture should define a value for MALLOC_ABI_ALIGNMENT, because otherwise the default is simply word aligned, which causes sub-optimal code, at least for the neon fpu.
>
> This simple patch fixes PR target/58065 by defining the MALLOC_ABI_ALIGNMENT as BIGGEST_ALIGNMNET.
>
> As a proof that this has indeed some subtle influence on the generated code
> I created a new test case: The function foo is called by bar, and bar uses
> malloc to allocate the memory, with compiler options "-O3 -g0 -mfpu=neon
> -mfloat-abi=softfp" the function foo is inlined into bar,
> but the inlined version does not use vstr instructions any more, because
> the front-end does assume that malloc returns 4 byte aligned memory.


You don't mention whether this patch was regression tested. All patches 
must be regression tested. I've applied only the changes to the source 
after testing it overnight with a couple of my other patches. Read 
further on about why this testcase is not suitable and how you could 
make this better for next time.

For next time please read the contribution web page with respect to 
testing here

http://gcc.gnu.org/contribute.html


> Regards
> Bernd Edlinger 		 	   		


Now moving on to the patch itself -

The testcase doesn't work because the vstr's that come out aren't 
because the alignment information doesn't match up rather than it being 
because the compiler has decided to use immediate offset addressing on 
storing a vector. Once you do that for a 128 bit vector there is no 
option but for the compiler to put out 2 vstr instructions which happens 
even after your patch for me.

> +++ gcc/testsuite/gcc.target/arm/pr58065.c	2013-08-03 00:08:22.000000000 +0200
> @@ -0,0 +1,25 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O3 -g0 -mfpu=neon -mfloat-abi=softfp" } */

For next time please use dg-add-options arm_neon for the -mfpu=neon 
-mfloat-abi=softfp bits. You also shouldn't need -g0 here.

Instead I'd scan an intermedate rtl dump for alignment information 
rather than anything else. I haven't yet looked at the output but 
looking for A64 in an RTL dump instead of A32 might help in this 
particular case.

Also reading http://gcc.gnu.org/wiki/TestCaseWriting will be useful with 
respect to creating new testcases.


> +/* { dg-final { scan-assembler-not "\[^v]str" } } */
> +
> +#include <stdlib.h>
> +
> +typedef long long V
> +  __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
> +
> +struct T { V a; V b[1]; };
> +
> +void
> +foo (struct T *p)
> +{
> +  V a = { 3, 4 };
> +  p->b[0] = a;
> +}
> +
> +struct T *
> +bar ()
> +{
> +  struct T *t = (struct T *) malloc (sizeof(*t));
> +  foo (t);
> +  return t;
> +}

Please post the changelog inline in the future.

> 2013-08-07  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>
> 	PR target/58065
> 	Set MALLOC_ABI_ALIGNMENT to BIGGEST_ALIGNMENT for ARM.
>
> 	* gcc/config/arm/arm.h: Define MALLOC_ABI_ALIGNMENT.
> 	* gcc/testsuite/gcc.target/arm/pr58065.c: New testcase.

Also for next time there should be a changelog entry for each ChangeLog 
file you need to touch in this case with the file names in the changelog 
entry rooted at the file in which the ChangeLog file is present. In this 
particular case you need one for the source base and one for the 
testsuite and drop the gcc/ and gcc/testsuite prefixes to the file names 
below. Also notice how I've changed the Changelog to actually refer to 
MALLOC_ABI_ALIGNMENT rather than stating what's changed for arm.h.


Therefore this should read


2013-08-08  Bernd Edlinger  <bernd.edlinger@hotmail.de>

 	PR target/58065
 	* config/arm/arm.h (MALLOC_ABI_ALIGNMENT): New,


2013-08-08  Bernd Edlinger  <bernd.edlinger@hotmail.de>

	PR target/58065
 	* gcc.target/arm/pr58065.c: New testcase.

This makes it easier for the person applying the patch to copy paste the 
Changelog into the relevant file. Thanks :)

And finally thank you for submitting the patch, if you intend on 
contributing to GCC in the medium to longer term, it would be worth 
getting the process for obtaining a copyright assignment sorted if you 
haven't done so already.

If you send a request for a copyright assignment form to 
gcc@gcc.gnu.org, a maintainer who has access to the relevant forms will 
forward this on to you.

regards
Ramana