Re: Allow passing arrays in registers on AArch64

[Michael Hudson-Doyle <michael dot hudson at canonical dot com>]

Ramana Radhakrishnan <ramana.gcc@googlemail.com> writes:

> On Tue, Feb 4, 2014 at 2:12 AM, Michael Hudson-Doyle
> <michael.hudson@linaro.org> wrote:
>> Ping?  I'm attaching a marginally cleaner version of the test.  I've had
>> a look at integrating this into aapcs64.exp but got defeated in the
>> end.  If go-torture-execute took a list of sources as c-torture-execute
>> does, then I think adding something like this to aapcs64.exp would work:
>>
>> # Test passing arrays by value
>> set src $srcdir/$subdir/test_array.go
>> if {[runtest_file_p $runtests $src]} {
>>     go-torture-execute [list $src $srcdir/$subdir/abitest.S $srcdir/$subdir/_test_array_c.c] \
>>         $additional_flags
>> }
>>
>> but it doesn't.  I also think that some stuff needs to be set up before
>> go-torture-execute can be called that I don't really understand and I
>> also also don't know how to avoid executing this test if gccgo hasn't
>> been built.
>
> Putting in a shell script like that is a bad idea, 

To be clear: I was never proposing that this shell script be committed.
It was just an unambiguous way of showing how to run my test.

> the dejagnu infrastructure can take care of all the transport and
> target bits for this. This will fail if someone were to try testing
> this on qemu while the rest of the testsuite might work.
>
> However what happens if in aarch64.exp
>
> you do a
>
> load_lib go-dg.exp
>
> and essentially run the tests similar to testsuite/go.dg/dg.exp  ?

I can't see how to run a test case that consists of multiple source
files like that.  That doesn't mean it's not possible though...

> That will take care of any cross-testing issues I hope with this using
> dejagnu.
>
> If that fails I think we should just drop the test as the go testsuite
> will catch this.

Please.

>>
>> All that said, is there any chance of getting the original ABI fix
>> committed?  It would be nice to have it in 4.9.
>
>
> I cannot approve or disapprove this patch but looking at the fix and
> the ABI issue under question here, I agree that this should be fixed
> for 4.9 and documented in the release notes. Your latest patch should
> also take Yufeng's suggestion under consideration about merging the
> condition in the if block.

Oh right, I forgot that I hadn't sent a patch acting on that comment.
Here is one.

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 16c51a8..958c667 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -1187,14 +1187,10 @@ aarch64_pass_by_reference (cumulative_args_t pcum ATTRIBUTE_UNUSED,
   size = (mode == BLKmode && type)
     ? int_size_in_bytes (type) : (int) GET_MODE_SIZE (mode);
 
-  if (type)
+  /* Aggregates are passed by reference based on their size.  */
+  if (type && AGGREGATE_TYPE_P (type))
     {
-      /* Arrays always passed by reference.  */
-      if (TREE_CODE (type) == ARRAY_TYPE)
-	return true;
-      /* Other aggregates based on their size.  */
-      if (AGGREGATE_TYPE_P (type))
-	size = int_size_in_bytes (type);
+      size = int_size_in_bytes (type);
     }
 
   /* Variable sized arguments are always returned by reference.  */

Cheers,
mwh

>
> Cheers,
> Ramana
>
>>
>> Cheers,
>> mwh
>>
>> Michael Hudson-Doyle <michael.hudson@linaro.org> writes:
>>
>>> Richard Earnshaw <rearnsha@arm.com> writes:
>>>
>>>> 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.
>>>
>>> Yeah, I had found those tests.  It would be much easier if I could write
>>> this in C :-)
>>>
>>>> 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.
>>>
>>> Right, that much makes sense.  I'm not sure I completely understand all
>>> the preprocessor trickery involved but the output of it seems clear
>>> enough.
>>>
>>>> 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.
>>>
>>> Well of course it cannot be done in C or C++.  A commenter on the PR
>>> said that while fortran does allow passing arrays by value, it's all
>>> handled in the frontend.  No idea about Java.  Ada has arrays by value
>>> but I don't know it even slightly.  So it's the last one on your list
>>> but here's a diff that adds hack at a test in Go:
>>>
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S b/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S
>>> index 86ce7be..365cd91 100644
>>> --- a/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S
>>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S
>>> @@ -1,9 +1,12 @@
>>>       .global dumpregs
>>>       .global myfunc
>>> +     .global main.myfunc
>>>       .type dumpregs,%function
>>>       .type myfunc,%function
>>> +     .type main.myfunc,%function
>>>  dumpregs:
>>>  myfunc:
>>> +main.myfunc:
>>>        mov    x16, sp
>>>        mov    x17, sp
>>>        sub    sp,  sp, 352 // 336 for registers and 16 for old sp and lr
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go
>>> new file mode 100644
>>> index 0000000..b5e90e4
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go
>>> @@ -0,0 +1,9 @@
>>> +package main
>>> +
>>> +func myfunc(e [2]int64)
>>> +
>>> +func main() {
>>> +     myfunc([2]int64{40, 50})
>>> +}
>>> +
>>> +
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh
>>> new file mode 100755
>>> index 0000000..0b3202d
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh
>>> @@ -0,0 +1,11 @@
>>> +#!/bin/sh
>>> +GCC=${GCC:-gcc}
>>> +AARCH64HOST=${AARCH64HOST:-localhost}
>>> +
>>> +set -x
>>> +
>>> +${GCC}go -g -c test_array.go -o test_array.o
>>> +${GCC} -g -c abitest.S -o abitest.o
>>> +${GCC} -g -c test_array_c.c -o test_array_c.o
>>> +${GCC}go -g abitest.o test_array.o test_array_c.o -o test_array
>>> +scp ./test_array ${AARCH64HOST}: && ssh ${AARCH64HOST} ./test_array
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c
>>> new file mode 100644
>>> index 0000000..981d12d
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c
>>> @@ -0,0 +1,19 @@
>>> +int which_kind_of_test = 0;
>>> +
>>> +#include "abitest-common.h"
>>> +
>>> +void
>>> +testfunc (char *stack)
>>> +{
>>> +  {
>>> +    long __x = 40;
>>> +    if (memcmp (&__x, stack + X0, sizeof (long)) != 0)
>>> +      abort ();
>>> +  }
>>> +  {
>>> +    long __x = 50;
>>> +    if (memcmp (&__x, stack + X1, sizeof (long)) != 0)
>>> +      abort ();
>>> +  }
>>> +  return;
>>> +}
>>>
>>> Obviously it's not integrated into the test framework even slightly but
>>> on the good side, it fails without my fix and passes with it... Are you
>>> able to do the integration part or provide some hints for me?
>>>
>>> Cheers,
>>> mwh
>>
>>
>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go
>> new file mode 100644
>> index 0000000..c795e88
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go
>> @@ -0,0 +1,10 @@
>> +package main
>> +
>> +//extern myfunc
>> +func myfunc(e [2]int64)
>> +
>> +func main() {
>> +       myfunc([2]int64{40, 50})
>> +}
>> +
>> +
>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh
>> new file mode 100755
>> index 0000000..1c41192
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh
>> @@ -0,0 +1,8 @@
>> +#!/bin/sh
>> +GCCGO=${GCCGO:-gccgo}
>> +AARCH64HOST=${AARCH64HOST:-localhost}
>> +
>> +set -x
>> +
>> +${GCCGO} -g test_array.go abitest.S test_array_c.o -o test_array
>> +scp ./test_array ${AARCH64HOST}: && ssh ${AARCH64HOST} ./test_array
>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c
>> new file mode 100644
>> index 0000000..41066b2
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c
>> @@ -0,0 +1,20 @@
>> +int which_kind_of_test = 0;
>> +
>> +#include "abitest-common.h"
>> +
>> +void
>> +testfunc (char *stack)
>> +{
>> +  {
>> +    long __x = 40;
>> +    if (memcmp (&__x, stack + X0, sizeof (long)) != 0)
>> +      abort ();
>> +  }
>> +  {
>> +    long __x = 50;
>> +    if (memcmp (&__x, stack + X1, sizeof (long)) != 0)
>> +      abort ();
>> +  }
>> +  return;
>> +}
>> +
>>