| Message ID | 87ha93nhl1.fsf@canonical.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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); }
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.