* [PATCH][AArch64] Wrong type-attribute for stp and str
@ 2017-10-16 13:27 Dominik Inführ
2017-10-20 14:41 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 8+ messages in thread
From: Dominik Inführ @ 2017-10-16 13:27 UTC (permalink / raw)
To: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
Hi,
it seems the type attributes for neon_stp and neon_store1_1reg<q> should be the other way around.
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")]
)
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
2017-10-16 13:27 [PATCH][AArch64] Wrong type-attribute for stp and str Dominik Inführ
@ 2017-10-20 14:41 ` Richard Earnshaw (lists)
2017-10-23 16:43 ` Dominik Inführ
0 siblings, 1 reply; 8+ messages in thread
From: Richard Earnshaw (lists) @ 2017-10-20 14:41 UTC (permalink / raw)
To: Dominik Inführ, GCC Patches
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")]
> )
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
2017-10-20 14:41 ` Richard Earnshaw (lists)
@ 2017-10-23 16:43 ` Dominik Inführ
2017-10-24 9:48 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 8+ messages in thread
From: Dominik Inführ @ 2017-10-23 16:43 UTC (permalink / raw)
To: Richard Earnshaw (lists); +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 3484 bytes --]
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")]
>> )
>>
>
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
2017-10-23 16:43 ` Dominik Inführ
@ 2017-10-24 9:48 ` Richard Earnshaw (lists)
2017-10-24 14:58 ` Dominik Inführ
0 siblings, 1 reply; 8+ messages in thread
From: Richard Earnshaw (lists) @ 2017-10-24 9:48 UTC (permalink / raw)
To: Dominik Inführ; +Cc: GCC Patches
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")]
>>> )
>>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
2017-10-24 9:48 ` Richard Earnshaw (lists)
@ 2017-10-24 14:58 ` Dominik Inführ
2017-10-24 14:58 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 8+ messages in thread
From: Dominik Inführ @ 2017-10-24 14:58 UTC (permalink / raw)
To: Richard Earnshaw (lists); +Cc: GCC Patches, Dr. Philipp Tomsich
[-- Attachment #1: Type: text/plain, Size: 5263 bytes --]
> 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")]
>>>> )
>>>>
>>>
>>
>
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
2017-10-24 14:58 ` Dominik Inführ
@ 2017-10-24 14:58 ` Richard Earnshaw (lists)
2017-10-30 17:59 ` Dominik Inführ
0 siblings, 1 reply; 8+ messages in thread
From: Richard Earnshaw (lists) @ 2017-10-24 14:58 UTC (permalink / raw)
To: Dominik Inführ; +Cc: GCC Patches, Dr. Philipp Tomsich
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")]
>>>>> )
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
2017-10-24 14:58 ` Richard Earnshaw (lists)
@ 2017-10-30 17:59 ` Dominik Inführ
0 siblings, 0 replies; 8+ messages in thread
From: Dominik Inführ @ 2017-10-30 17:59 UTC (permalink / raw)
To: Richard Earnshaw (lists); +Cc: GCC Patches, Dr. Philipp Tomsich
[-- Attachment #1: Type: text/plain, Size: 5880 bytes --]
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")]
>>>>>> )
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH][AArch64] Wrong type-attribute for stp and str
@ 2017-10-30 18:42 Wilco Dijkstra
0 siblings, 0 replies; 8+ messages in thread
From: Wilco Dijkstra @ 2017-10-30 18:42 UTC (permalink / raw)
To: dominik.infuehr; +Cc: nd, Richard Earnshaw, GCC Patches, philipp.tomsich
Dominik wrote:
> Could you please also commit the patch? I don’t have commit rights.
I've committed it as r254236.
Wilco
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-10-30 18:38 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-16 13:27 [PATCH][AArch64] Wrong type-attribute for stp and str Dominik Inführ
2017-10-20 14:41 ` Richard Earnshaw (lists)
2017-10-23 16:43 ` Dominik Inführ
2017-10-24 9:48 ` Richard Earnshaw (lists)
2017-10-24 14:58 ` Dominik Inführ
2017-10-24 14:58 ` Richard Earnshaw (lists)
2017-10-30 17:59 ` Dominik Inführ
2017-10-30 18:42 Wilco Dijkstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).