| Message ID | A17B0A02-3A67-4F63-924C-0B25621BCDA3@theobroma-systems.com |
|---|---|
| State | New |
| Headers | show |
| Series | [AArch64] Wrong type-attribute for stp and str | expand |
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.infuehr@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")] > ) >
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.infuehr@theobroma-systems.com>
* config/aarch64/aarch64-simd.md
(*aarch64_simd_mov<VD:mode>): Fix type-attribute.
(*aarch64_simd_mov<VQ:mode>): Likewise.
—
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.Earnshaw@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.infuehr@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")]
>> )
>>
>
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.infuehr@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.Earnshaw@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.infuehr@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")] >>> ) >>> >> >
> On 24 Oct 2017, at 11:40, Richard Earnshaw (lists) <Richard.Earnshaw@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.infuehr@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'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.infuehr@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.Earnshaw@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.infuehr@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")] >>>> ) >>>> >>> >> >
On 24/10/17 15:54, Dominik Inführ wrote: > >> On 24 Oct 2017, at 11:40, Richard Earnshaw (lists) <Richard.Earnshaw@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.infuehr@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.infuehr@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.Earnshaw@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.infuehr@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")] >>>>> ) >>>>> >>>> >>> >> >
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.Earnshaw@arm.com> wrote: > > On 24/10/17 15:54, Dominik Inführ wrote: >> >>> On 24 Oct 2017, at 11:40, Richard Earnshaw (lists) <Richard.Earnshaw@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.infuehr@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.infuehr@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.Earnshaw@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.infuehr@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")] >>>>>> )
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")] )