Re: New patch: [AArch64] [BE] [1/2] Make large opaque integer modes endianness-safe.

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 15 December 2014 at 10:56, David Sherwood <david.sherwood@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Christophe Lyon [mailto:christophe.lyon@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.sherwood@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.sherwood@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.sherwood@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.
>
OK thanks for the clarification.


>> >> 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.sherwood@arm.com>
>     2014-10-10  Tejas Belagod <Tejas.Belagod@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.sherwood@arm.com>
>     2014-13-10  Tejas Belagod <Tejas.Belagod@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.
>> >
>> >
>> >
>> >
>
>
>
>