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")] >>>>> ) >>>>> >>>> >>> >> >
