> 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'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
