> -----Original Message----- > From: Christophe Lyon [mailto:christophe.l...@linaro.org] > Sent: 11 December 2014 13:47 > To: David Sherwood > Cc: gcc-patches@gcc.gnu.org; Marcus Shawcroft; Alan Hayward; Tejas Belagod; > Richard Sandiford > Subject: Re: New patch: [AArch64] [BE] [1/2] Make large opaque integer modes > endianness-safe. > > On 11 December 2014 at 11:16, David Sherwood <david.sherw...@arm.com> wrote: > > Hi Christophe, > > > > Sorry to bother you again. After my clarification email below are you now > > happy for these patches to go in? > > > > Kind Regards, > > David Sherwood. > > > >> -----Original Message----- > >> From: David Sherwood [mailto:david.sherw...@arm.com] > >> Sent: 27 November 2014 14:53 > >> To: 'Christophe Lyon' > >> Cc: gcc-patches@gcc.gnu.org; Marcus Shawcroft; Alan Hayward; 'Tejas > >> Belagod'; Richard Sandiford > >> Subject: RE: New patch: [AArch64] [BE] [1/2] Make large opaque integer > >> modes endianness-safe. > >> > >> > On 18 November 2014 10:14, David Sherwood <david.sherw...@arm.com> wrote: > >> > > Hi Christophe, > >> > > > >> > > Ah sorry. My mistake - it fixes this in bugzilla: > >> > > > >> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59810 > >> > > >> > I did look at that PR, but since it has no testcase attached, I was > >> > unsure. > >> > And I am still not :-) > >> > PR 59810 is "[AArch64] LDn/STn implementations are not ABI-conformant > >> > for bigendian." > >> > but the advsimd-intrinsics/vldX.c and vldX_lane.c now PASS with Alan's > >> > patches on aarch64_be, so I thought Alan's patches solve PR59810. > >> > > >> > What am I missing? > >> > >> Hi Christophe, > >> > >> I think probably this is our fault for making our lives way too difficult > >> and > >> artificially splitting all these patches up. :) > >> > >> Alan's patch: > >> > >> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00952.html > >> > >> fixes some issues on aarch64_be, but also causes regressions. For example, > >> > >> ==== > >> Tests that now fail, but worked before: > >> > >> aarch64_be-elf-aem: gcc.dg/vect/slp-perm-8.c -flto -ffat-lto-objects > >> execution test > >> aarch64_be-elf-aem: gcc.dg/vect/slp-perm-8.c execution test > >> aarch64_be-elf-aem: gcc.dg/vect/vect-over-widen-1-big-array.c -flto > >> -ffat-lto-objects execution test > >> ... > >> > >> Tests that now work, but didn't before: > >> > >> aarch64_be-elf-aem: gcc.dg/vect/fast-math-vect-complex-3.c execution test > >> aarch64_be-elf-aem: gcc.dg/vect/if-cvt-stores-vect-ifcvt-18.c execution > >> test > >> aarch64_be-elf-aem: gcc.dg/vect/no-scevccp-outer-10a.c execution test > >> ... > >> ==== > I didn't notice that because I tested Alan's patch only against the > advsimd-intrinsics tests. > In this respect, I don't understand why your ChangeLog entry says > * config/aarch64/aarch64-simd.md (vec_store_lanes(o/c/x)i, > vec_load_lanes(o/c/x)i): Fixed to work for Big Endian. > since the existing advsimd-intrinsics tests already pass with Alan's patch > alone > or is vld1_lane still broken (for which I haven't posted a test yet)? > Yes, I think the change log is unclear and I will change it. The only thing that was broken was not adhering to the ABI, but we don't have any specific regression tests that prove this.
> >> His patch is only half of the story and must be applied at the same time > >> as the > >> "[AArch64] [BE] [1/2] Make large opaque integer modes endianness-safe." > >> patch. With both patches applied the result looks much healthier: > >> > >> ==== > >> # Comparing 1 common sum files > >> ## /bin/sh ./src/gcc/contrib/compare_tests /tmp/gxx-sum1.10051 > >> /tmp/gxx-sum2.10051 > >> Tests that now work, but didn't before: > >> > >> aarch64_be-elf-aem: gcc.dg/torture/pr52028.c -O3 -fomit-frame-pointer > >> execution test > >> aarch64_be-elf-aem: gcc.dg/torture/pr52028.c -O3 -fomit-frame-pointer > >> -funroll-all-loops -finline- > >> functions execution test > >> aarch64_be-elf-aem: gcc.dg/torture/pr52028.c -O3 -fomit-frame-pointer > >> -funroll-loops execution > test > >> ... > >> ==== > >> > >> with no new regressions. After applying both patches the aarch64_be gcc > >> testsuite is > >> on a parity with the aarch64 testsuite. Furthermore, after applying both > >> of these patches: > >> > >> "[AArch64] [BE] [1/2] Make large opaque integer modes endianness-safe" > >> "[AArch64] [BE] Fix vector load/stores to not use ld1/st1" > >> > >> it then becomes safe for us to remove the CCMC macro, which is the cause of > >> unnecessary spills to the stack for certain auto-vectorised code. So > >> really I > >> suppose when I posted my second patch > >> > >> "[AArch64] [BE] [2/2] Make large opaque integer modes endianness-safe" > >> > >> I should have really just called this > >> > >> "[AArch64] [BE] Remove CCMC for aarch64" > >> > >> in order to make it clear exactly what the purpose of these patches is. > well, not yet since this very does not remove it :-) > Again, this is my fault as I made a mistake in the change log. If you look at the actual patch the CCMC macro is removed. Let me re-post corrected, more sensible change logs for both of those changes here: "[AArch64] [BE] [1/2] Make large opaque integer modes endianness-safe" ChangeLog: gcc/: 2014-10-10 David Sherwood <david.sherw...@arm.com> 2014-10-10 Tejas Belagod <tejas.bela...@arm.com> * config/aarch64/aarch64-protos.h (aarch64_simd_attr_length_rglist, aarch64_reverse_mask): New decls. * config/aarch64/iterators.md (UNSPEC_REV_REGLIST): New enum. * config/aarch64/iterators.md (insn_count): New mode_attr. * config/aarch64/aarch64-simd.md (vec_store_lanesoi, vec_store_lanesci, vec_store_lanesxi, vec_load_lanesoi, vec_load_lanesci, vec_load_lanesxi) : Made ABI compliant for Big Endian targets. * config/aarch64/aarch64-simd.md (aarch64_rev_reglist, aarch64_simd_ld2, aarch64_simd_ld3, aarch64_simd_ld4, aarch64_simd_st2, aarch64_simd_st3, aarch64_simd_st4): Added. * config/aarch64/aarch64.c (aarch64_simd_attr_length_rglist, aarch64_reverse_mask): Added. "[AArch64] [BE] [2/2] Make large opaque integer modes endianness-safe" ChangeLog: gcc/: 2014-13-10 David Sherwood <david.sherw...@arm.com> 2014-13-10 Tejas Belagod <tejas.bela...@arm.com> * config/aarch64/aarch64.h (CANNOT_CHANGE_MODE_CLASS): Removed. * config/aarch64/aarch64.c (aarch64_cannot_change_mode_class): Removed. * config/aarch64/aarch64-protos.h (aarch64_cannot_change_mode_class): Removed. Again, apologies for the confusion, David. > >> > >> Kind Regards, > >> David Sherwood. > > > > > > > >