Could you please also commit the patch? I don’t have commit rights. Best Dominik
> On 24 Oct 2017, at 16:58, Richard Earnshaw (lists) <richard.earns...@arm.com> > wrote: > > On 24/10/17 15:54, Dominik Inführ wrote: >> >>> On 24 Oct 2017, at 11:40, Richard Earnshaw (lists) >>> <richard.earns...@arm.com> wrote: >>> >>> On 23/10/17 17:36, Dominik Inführ wrote: >>>> I’ve added your suggestions. I would also like to propose to change the >>>> type attribute from neon_stp to store_8 and store_16, this seems to be >>>> more in line with respect to other patterns. >>>> >>>> Thanks, >>>> Dominik >>>> >>>> ChangeLog: >>>> 2017-10-23 Dominik Infuehr <dominik.infu...@theobroma-systems.com> >>>> >>>> * config/aarch64/aarch64-simd.md >>>> (*aarch64_simd_mov<VD:mode>): Fix type-attribute. >>>> (*aarch64_simd_mov<VQ:mode>): Likewise. >>> >>> I don't think we should conflate loads/stores to the simd registers with >>> loads/stores to the gp registers. They might have very different >>> characteristics. So no, I don't think we should change it that way. >> >> I agree, but I don’t think my changes would conflate that. I only changed >> the types for the pattern alternatives `str xzr, %1` and `stp xzr, xzr, %1` >> to store_8 and store_16 since both instructions don’t actually involve >> SIMD/FP-registers. Using neon_stp for both `stp xzr, xzr, %0` and `stp %d1, >> %d3, %0` seems misleading because of possibly different characteristics. Am >> I missing something? > > You're not missing anything. I looking at an old version of the code > and getting confused :-( > > OK. > > R. > >> >>> >>> You've also missed the renaming of the ambiguous patterns from your >>> changelog entry. Finally, 'fix xxx' is generally frowned upon in >>> ChangeLogs. A better description would be 'Re-order type attributes to >>> match pattern alternatives’. >> >> Ok, thanks for pointing that out. Here is the updated ChangeLog: >> >> 2017-10-24 Dominik Infuehr <dominik.infu...@theobroma-systems.com> >> >> * config/aarch64/aarch64-simd.md (*aarch64_simd_mov): Rename >> both identically named patterns to (*aarch64_simd_mov<VD:mode>) >> and (*aarch64_simd_mov<VQ:mode>). >> (*aarch64_simd_mov<VD:mode>): Change type attribute to match >> pattern alternative. >> (*aarch64_simd_mov<VQ:mode>): Re-order and change type >> attributes to match pattern alternative. >> >>> >>> R. >>> >>>> — >>>> diff --git a/gcc/config/aarch64/aarch64-simd.md >>>> b/gcc/config/aarch64/aarch64-simd.md >>>> index 49f615cfdbf..447ee3afd17 100644 >>>> --- a/gcc/config/aarch64/aarch64-simd.md >>>> +++ b/gcc/config/aarch64/aarch64-simd.md >>>> @@ -102,7 +102,7 @@ >>>> [(set_attr "type" "neon_dup<q>")] >>>> ) >>>> >>>> -(define_insn "*aarch64_simd_mov<mode>" >>>> +(define_insn "*aarch64_simd_mov<VD:mode>" >>>> [(set (match_operand:VD 0 "nonimmediate_operand" >>>> "=w, m, m, w, ?r, ?w, ?r, w") >>>> (match_operand:VD 1 "general_operand" >>>> @@ -126,12 +126,12 @@ >>>> default: gcc_unreachable (); >>>> } >>>> } >>>> - [(set_attr "type" "neon_load1_1reg<q>, neon_stp, neon_store1_1reg<q>,\ >>>> + [(set_attr "type" "neon_load1_1reg<q>, store_8, neon_store1_1reg<q>,\ >>>> neon_logic<q>, neon_to_gp<q>, f_mcr,\ >>>> mov_reg, neon_move<q>")] >>>> ) >>>> >>>> -(define_insn "*aarch64_simd_mov<mode>" >>>> +(define_insn "*aarch64_simd_mov<VQ:mode>" >>>> [(set (match_operand:VQ 0 "nonimmediate_operand" >>>> "=w, Umq, m, w, ?r, ?w, ?r, w") >>>> (match_operand:VQ 1 "general_operand" >>>> @@ -160,8 +160,8 @@ >>>> gcc_unreachable (); >>>> } >>>> } >>>> - [(set_attr "type" "neon_load1_1reg<q>, neon_store1_1reg<q>,\ >>>> - neon_stp, neon_logic<q>, multiple, multiple,\ >>>> + [(set_attr "type" "neon_load1_1reg<q>, store_16, neon_store1_1reg<q>,\ >>>> + neon_logic<q>, multiple, multiple,\ >>>> multiple, neon_move<q>") >>>> (set_attr "length" "4,4,4,4,8,8,8,4")] >>>> ) >>>> >>>> >>>>> On 20 Oct 2017, at 16:07, Richard Earnshaw (lists) >>>>> <richard.earns...@arm.com> wrote: >>>>> >>>>> On 16/10/17 14:26, Dominik Inführ wrote: >>>>>> Hi, >>>>>> >>>>>> it seems the type attributes for neon_stp and neon_store1_1reg<q> should >>>>>> be the other way around. >>>>>> >>>>> >>>>> Yes, I agree, but there's more.... >>>>> >>>>> Firstly, we have two patterns that are named *aarch64_simd_mov<mode>, >>>>> with different iterators. That's slightly confusing. I think they need >>>>> to be renamed as: >>>>> >>>>> *aarch64_simd_mov<VD:mode> >>>>> >>>>> and >>>>> >>>>> *aarch64_simd_mov<VQ:mode> >>>>> >>>>> to break the ambiguity. >>>>> >>>>> Secondly it looks to me as though the attributes on the other one are >>>>> also incorrect. Could you check that one out as well, please. >>>>> >>>>> Thanks, >>>>> >>>>> R. >>>>> >>>>>> Thanks >>>>>> Dominik >>>>>> >>>>>> ChangeLog: >>>>>> 2017-10-16 Dominik Infuehr <dominik.infu...@theobroma-systems.com> >>>>>> >>>>>> * config/aarch64/aarch64-simd.md >>>>>> (*aarch64_simd_mov<mode>): Fix type-attribute. >>>>>> -- >>>>>> diff --git a/gcc/config/aarch64/aarch64-simd.md >>>>>> b/gcc/config/aarch64/aarch64-simd.md >>>>>> index 49f615cfdbf..409ad3502ff 100644 >>>>>> --- a/gcc/config/aarch64/aarch64-simd.md >>>>>> +++ b/gcc/config/aarch64/aarch64-simd.md >>>>>> @@ -160,8 +160,8 @@ >>>>>> gcc_unreachable (); >>>>>> } >>>>>> } >>>>>> - [(set_attr "type" "neon_load1_1reg<q>, neon_store1_1reg<q>,\ >>>>>> - neon_stp, neon_logic<q>, multiple, multiple,\ >>>>>> + [(set_attr "type" "neon_load1_1reg<q>, neon_stp, neon_store1_1reg<q>,\ >>>>>> + neon_logic<q>, multiple, multiple,\ >>>>>> multiple, neon_move<q>") >>>>>> (set_attr "length" "4,4,4,4,8,8,8,4")] >>>>>> )
signature.asc
Description: Message signed with OpenPGP using GPGMail
