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