Patchwork Allow passing arrays in registers on AArch64

login
register
mail settings
Submitter Michael Hudson-Doyle
Date Jan. 17, 2014, 8:22 a.m.
Message ID <87ha93nhl1.fsf@canonical.com>
Download mbox | patch
Permalink /patch/311982/
State New
Headers show

Comments

Michael Hudson-Doyle - Jan. 17, 2014, 8:22 a.m.
Hi, as discussed in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59799
GCC currently gets a detail of the AArch64 ABI wrong: arrays are not
always passed by reference.  Fortunately the fix is rather easy...

I guess this is an ABI break but my understand there has been no release
of GCC which supports compiling a language that can pass arrays by value
on AArch64 yet.

Cheers,
mwh

  2014-01-17  Michael Hudson-Doyle  <michael.hudson@linaro.org>

            PR target/59799

            * config/aarch64/aarch64.c (aarch64_pass_by_reference):
              The rules for passing arrays in registers are the same as
              for structs, so remove the special case for them.
Yufeng Zhang - Jan. 17, 2014, 6:50 p.m.
Hi Michael,

Thanks for the fix.  The patch looks OK to me in general, although I 
have some minor comments below.

On 01/17/14 08:22, Michael Hudson-Doyle wrote:
> Hi, as discussed inhttp://gcc.gnu.org/bugzilla/show_bug.cgi?id=59799
> GCC currently gets a detail of the AArch64 ABI wrong: arrays are not
> always passed by reference.  Fortunately the fix is rather easy...

Can you please indicate what kind of testing you have run, e.g. regtest 
on aarch64-none-abi?

Also can you please try to add some new test(s)?  It may not be that 
straightforward to add non-C/C++ tests, but give it a try.

>
> I guess this is an ABI break but my understand there has been no release
> of GCC which supports compiling a language that can pass arrays by value
> on AArch64 yet.
>
> Cheers,
> mwh
>
>    2014-01-17  Michael Hudson-Doyle<michael.hudson@linaro.org>
>
>              PR target/59799
>
>              * config/aarch64/aarch64.c (aarch64_pass_by_reference):
>                The rules for passing arrays in registers are the same as
>                for structs, so remove the special case for them.
>
>
> aarch64-abi-fix.diff
>
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index fa53c71..d63da95 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -987,10 +987,7 @@ aarch64_pass_by_reference (cumulative_args_t pcum ATTRIBUTE_UNUSED,
>
>     if (type)
>       {
> -      /* Arrays always passed by reference.  */
> -      if (TREE_CODE (type) == ARRAY_TYPE)
> -	return true;
> -      /* Other aggregates based on their size.  */
> +      /* Aggregates based on their size.  */
>         if (AGGREGATE_TYPE_P (type))
>   	size = int_size_in_bytes (type);
>       }
>

You can actually merge the two iffs to have something like:

   /* Aggregates are based on their size.  */
   if (type && AGGREGATE_TYPE_P (type))
     size = int_size_in_bytes (type);

Thanks,
Yufeng
Ian Lance Taylor - Jan. 17, 2014, 7:43 p.m.
On Fri, Jan 17, 2014 at 11:32 AM, Michael Hudson-Doyle
<michael.hudson@canonical.com> wrote:
>
> On 18 Jan 2014 07:50, "Yufeng Zhang" <Yufeng.Zhang@arm.com> wrote:
>>
>> Also can you please try to add some new test(s)?  It may not be that
>> straightforward to add non-C/C++ tests, but give it a try.
>
> Can you give some hints? Like at least where in the tree such a test would
> go? I don't know this code at all.

There is already a test in libgo, of course.

I think it would be pretty hard to write a test that doesn't something
like what libgo does.  The problem is that GCC is entirely consistent
with and without your patch.  You could add a Go test that passes an
array in gcc/testsuite/go.go-torture/execute/ easily enough, but it
would be quite hard to add a test that doesn't pass whether or not
your patch is applied.

Ian
Michael Hudson-Doyle - Jan. 17, 2014, 11:56 p.m.
Ian Lance Taylor <iant@golang.org> writes:

> On Fri, Jan 17, 2014 at 11:32 AM, Michael Hudson-Doyle
> <michael.hudson@canonical.com> wrote:
>>
>> On 18 Jan 2014 07:50, "Yufeng Zhang" <Yufeng.Zhang@arm.com> wrote:
>>>
>>> Also can you please try to add some new test(s)?  It may not be that
>>> straightforward to add non-C/C++ tests, but give it a try.
>>
>> Can you give some hints? Like at least where in the tree such a test would
>> go? I don't know this code at all.
>
> There is already a test in libgo, of course.
>
> I think it would be pretty hard to write a test that doesn't something
> like what libgo does.  The problem is that GCC is entirely consistent
> with and without your patch.  You could add a Go test that passes an
> array in gcc/testsuite/go.go-torture/execute/ easily enough, but it
> would be quite hard to add a test that doesn't pass whether or not
> your patch is applied.

I think it would have to be a code generation test, i.e. that compiling
something like

func second(e [2]int64) int64 {
        return e[1]
}

does not access memory or something along those lines.  I'll have a look
next week.

Cheers,
mwh
Richard Earnshaw - Jan. 20, 2014, 9:50 a.m.
On 17/01/14 23:56, Michael Hudson-Doyle wrote:
> Ian Lance Taylor <iant@golang.org> writes:
> 
>> On Fri, Jan 17, 2014 at 11:32 AM, Michael Hudson-Doyle
>> <michael.hudson@canonical.com> wrote:
>>>
>>> On 18 Jan 2014 07:50, "Yufeng Zhang" <Yufeng.Zhang@arm.com> wrote:
>>>>
>>>> Also can you please try to add some new test(s)?  It may not be that
>>>> straightforward to add non-C/C++ tests, but give it a try.
>>>
>>> Can you give some hints? Like at least where in the tree such a test would
>>> go? I don't know this code at all.
>>
>> There is already a test in libgo, of course.
>>
>> I think it would be pretty hard to write a test that doesn't something
>> like what libgo does.  The problem is that GCC is entirely consistent
>> with and without your patch.  You could add a Go test that passes an
>> array in gcc/testsuite/go.go-torture/execute/ easily enough, but it
>> would be quite hard to add a test that doesn't pass whether or not
>> your patch is applied.
> 
> I think it would have to be a code generation test, i.e. that compiling
> something like
> 
> func second(e [2]int64) int64 {
>         return e[1]
> }
> 
> does not access memory or something along those lines.  I'll have a look
> next week.
> 
> Cheers,
> mwh
> 

Michael,

Can you have a look at how the tests in gcc.target/aarch64/aapcs64 work;
it ought to be possible to write a test (though not in C) to catch this.

The tests work by calling a wrapper function written in assembler --
that wrapper stores out all the registers used for parameter passing and
then calls back into the test's validator with the register dump
available.  Code can then check that values are passed in the places
expected.  This gives the compiler the separation between caller and
callee that's needed to test this feature.

My preferred languages for these tests would be (in approximate order)
c, c++, fortran, java, ada, go.  That order is based on which languages
are tested most by users.

R.
Matthias Klose - Jan. 24, 2014, 11:54 a.m.
Am 17.01.2014 19:50, schrieb Yufeng Zhang:
> Hi Michael,
> 
> Thanks for the fix.  The patch looks OK to me in general, although I have some
> minor comments below.
> 
> On 01/17/14 08:22, Michael Hudson-Doyle wrote:
>> Hi, as discussed inhttp://gcc.gnu.org/bugzilla/show_bug.cgi?id=59799
>> GCC currently gets a detail of the AArch64 ABI wrong: arrays are not
>> always passed by reference.  Fortunately the fix is rather easy...
> 
> Can you please indicate what kind of testing you have run, e.g. regtest on
> aarch64-none-abi?

Test with the trunk (all languages except Ada) with and without this patch:

 Running target unix
 FAIL: math
 FAIL: net
-FAIL: reflect
 FAIL: runtime

                === libgo Summary ===

-# of expected passes           118
-# of unexpected failures       4
+# of expected passes           119
+# of unexpected failures       3
 /home/doko/gcc/gcc-4.9-4.9-20140122/build/./gcc/gccgo version 4.9.0 20140122
(experimental) [trunk revision 206940] (Ubuntu 4.9-20140122-1ubuntu1)

no other changes for the tests.

I did test with the Linaro 4.8 branch, including FSF changes up to r206935, with
and without this patch:

-FAIL: io
-FAIL: os
-FAIL: reflect
-FAIL: sync
-FAIL: time
-FAIL: archive/zip
-FAIL: database/sql
-FAIL: encoding/base64
-FAIL: go/doc
-FAIL: go/printer
-FAIL: log/syslog
-FAIL: net/http/cgi
-FAIL: net/http/httputil
-FAIL: net/rpc
-FAIL: net/rpc/jsonrpc
-FAIL: old/template
-FAIL: os/exec
-FAIL: os/signal
-FAIL: sync/atomic
-FAIL: text/template

-# of expected passes           99
-# of unexpected failures       23
+# of expected passes           119
+# of unexpected failures       3

there are some XPASS's for quality tests, and FAIL's for the pr36728 quality
tests, however in the past these were always a bit noisy, succeeding or failing
for various builds.

I do see additional fails for:

+FAIL: gcc.dg/simulate-thread/atomic-other-int.c  -O0 -g  thread simulation test
+FAIL: gcc.dg/simulate-thread/atomic-other-int.c  -O2 -g  thread simulation test
+FAIL: gcc.dg/simulate-thread/atomic-other-int.c  -O3 -g  thread simulation test
+FAIL: gcc.dg/simulate-thread/atomic-other-short.c  -O0 -g  thread simulation test
+FAIL: gcc.dg/simulate-thread/atomic-other-short.c  -O2 -g  thread simulation test
+FAIL: gcc.dg/simulate-thread/atomic-other-short.c  -O3 -g  thread simulation test

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index fa53c71..d63da95 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -987,10 +987,7 @@  aarch64_pass_by_reference (cumulative_args_t pcum ATTRIBUTE_UNUSED,
 
   if (type)
     {
-      /* Arrays always passed by reference.  */
-      if (TREE_CODE (type) == ARRAY_TYPE)
-	return true;
-      /* Other aggregates based on their size.  */
+      /* Aggregates based on their size.  */
       if (AGGREGATE_TYPE_P (type))
 	size = int_size_in_bytes (type);
     }