This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][AArch64] Wrong type-attribute for stp and str
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")]
>>>>> )
>>>>>
>>>>
>>>
>>
>