[Patch AArch64] Fix extended register width

[Patch AArch64] Fix extended register width

[Carrot Wei]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

Hi

The extended register width in add/adds/sub/subs/cmp instructions is
not always the same as target register, it depends on both target
register width and extension type. But in current implementation the
extended register width is always the same as target register. We have
noticed it can generate following wrong assembler code when compiled
an internal application,

add     x2, x20, x0, sxtw 3

The correct assembler should be

add     x2, x20, w0, sxtw 3

On the other hand I noticed current gcc can only generate following
extension types: xtb, xth, xtw. In these cases the extended register
width can only be 'w'. So this patch changes the the extended register
size attribute to 'w'.

Passed regression tests on qemu without failure.
OK for trunk and 4.9 branch?

thanks
Guozhi Wei


2014-09-22  Guozhi Wei  <carrot@google.com>

        * config/aarch64/aarch64.md (*adds_<optab><ALLX:mode>_<GPI:mode>):
        Change the extended register width to w.
        (*subs_<optab><ALLX:mode>_<GPI:mode>): Likewise.
        (*adds_<optab><mode>_multp2): Likewise.
        (*subs_<optab><mode>_multp2): Likewise.
        (*add_<optab><ALLX:mode>_<GPI:mode>): Likewise.
        (*add_<optab><ALLX:mode>_shft_<GPI:mode>): Likewise.
        (*add_<optab><ALLX:mode>_mult_<GPI:mode>): Likewise.
        (*add_<optab><mode>_multp2): Likewise.
        (*add_uxt<mode>_multp2): Likewise.
        (*sub_<optab><ALLX:mode>_<GPI:mode>): Likewise.
        (*sub_<optab><ALLX:mode>_shft_<GPI:mode>): Likewise.
        (*sub_<optab><mode>_multp2): Likewise.
        (*sub_uxt<mode>_multp2): Likewise.
        (*cmp_swp_<optab><ALLX:mode>_reg<GPI:mode>): Likewise.
        (*cmp_swp_<optab><ALLX:mode>_shft_<GPI:mode>): Likewise.


2014-09-22  Guozhi Wei  <carrot@google.com>

        * gcc.target/aarch64/subs3.c: Change the extended register width to w.
        * gcc.target/aarch64/adds3.c: Likewise.
        * gcc.target/aarch64/cmp.c: Likewise.

[-- Attachment #2: patch1 --]
[-- Type: application/octet-stream, Size: 4455 bytes --]

Index: aarch64.md
===================================================================
--- aarch64.md	(revision 215364)
+++ aarch64.md	(working copy)
@@ -1336,7 +1336,7 @@
    (set (match_operand:GPI 0 "register_operand" "=r")
 	(plus:GPI (ANY_EXTEND:GPI (match_dup 1)) (match_dup 2)))]
   ""
-  "adds\\t%<GPI:w>0, %<GPI:w>2, %<GPI:w>1, <su>xt<ALLX:size>"
+  "adds\\t%<GPI:w>0, %<GPI:w>2, %w1, <su>xt<ALLX:size>"
   [(set_attr "type" "alus_ext")]
 )
 
@@ -1350,7 +1350,7 @@
    (set (match_operand:GPI 0 "register_operand" "=r")
 	(minus:GPI (match_dup 1) (ANY_EXTEND:GPI (match_dup 2))))]
   ""
-  "subs\\t%<GPI:w>0, %<GPI:w>1, %<GPI:w>2, <su>xt<ALLX:size>"
+  "subs\\t%<GPI:w>0, %<GPI:w>1, %w2, <su>xt<ALLX:size>"
   [(set_attr "type" "alus_ext")]
 )
 
@@ -1370,7 +1370,7 @@
 				   (const_int 0))
 		  (match_dup 4)))]
   "aarch64_is_extend_from_extract (<MODE>mode, operands[2], operands[3])"
-  "adds\\t%<w>0, %<w>4, %<w>1, <su>xt%e3 %p2"
+  "adds\\t%<w>0, %<w>4, %w1, <su>xt%e3 %p2"
   [(set_attr "type" "alus_ext")]
 )
 
@@ -1390,7 +1390,7 @@
 				  (match_dup 3)
 				  (const_int 0))))]
   "aarch64_is_extend_from_extract (<MODE>mode, operands[2], operands[3])"
-  "subs\\t%<w>0, %<w>4, %<w>1, <su>xt%e3 %p2"
+  "subs\\t%<w>0, %<w>4, %w1, <su>xt%e3 %p2"
   [(set_attr "type" "alus_ext")]
 )
 
@@ -1455,7 +1455,7 @@
 	(plus:GPI (ANY_EXTEND:GPI (match_operand:ALLX 1 "register_operand" "r"))
 		  (match_operand:GPI 2 "register_operand" "r")))]
   ""
-  "add\\t%<GPI:w>0, %<GPI:w>2, %<GPI:w>1, <su>xt<ALLX:size>"
+  "add\\t%<GPI:w>0, %<GPI:w>2, %w1, <su>xt<ALLX:size>"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1477,7 +1477,7 @@
 			      (match_operand 2 "aarch64_imm3" "Ui3"))
 		  (match_operand:GPI 3 "register_operand" "r")))]
   ""
-  "add\\t%<GPI:w>0, %<GPI:w>3, %<GPI:w>1, <su>xt<ALLX:size> %2"
+  "add\\t%<GPI:w>0, %<GPI:w>3, %w1, <su>xt<ALLX:size> %2"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1501,7 +1501,7 @@
 			    (match_operand 2 "aarch64_pwr_imm3" "Up3"))
 		  (match_operand:GPI 3 "register_operand" "r")))]
   ""
-  "add\\t%<GPI:w>0, %<GPI:w>3, %<GPI:w>1, <su>xt<ALLX:size> %p2"
+  "add\\t%<GPI:w>0, %<GPI:w>3, %w1, <su>xt<ALLX:size> %p2"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1526,7 +1526,7 @@
 		   (const_int 0))
 		  (match_operand:GPI 4 "register_operand" "r")))]
   "aarch64_is_extend_from_extract (<MODE>mode, operands[2], operands[3])"
-  "add\\t%<w>0, %<w>4, %<w>1, <su>xt%e3 %p2"
+  "add\\t%<w>0, %<w>4, %w1, <su>xt%e3 %p2"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1660,7 +1660,7 @@
   "*
   operands[3] = GEN_INT (aarch64_uxt_size (exact_log2 (INTVAL (operands[2])),
 					   INTVAL (operands[3])));
-  return \"add\t%<w>0, %<w>4, %<w>1, uxt%e3 %p2\";"
+  return \"add\t%<w>0, %<w>4, %w1, uxt%e3 %p2\";"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1811,7 +1811,7 @@
 		   (ANY_EXTEND:GPI
 		    (match_operand:ALLX 2 "register_operand" "r"))))]
   ""
-  "sub\\t%<GPI:w>0, %<GPI:w>1, %<GPI:w>2, <su>xt<ALLX:size>"
+  "sub\\t%<GPI:w>0, %<GPI:w>1, %w2, <su>xt<ALLX:size>"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1834,7 +1834,7 @@
 				(match_operand:ALLX 2 "register_operand" "r"))
 			       (match_operand 3 "aarch64_imm3" "Ui3"))))]
   ""
-  "sub\\t%<GPI:w>0, %<GPI:w>1, %<GPI:w>2, <su>xt<ALLX:size> %3"
+  "sub\\t%<GPI:w>0, %<GPI:w>1, %w2, <su>xt<ALLX:size> %3"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1860,7 +1860,7 @@
 		    (match_operand 3 "const_int_operand" "n")
 		    (const_int 0))))]
   "aarch64_is_extend_from_extract (<MODE>mode, operands[2], operands[3])"
-  "sub\\t%<w>0, %<w>4, %<w>1, <su>xt%e3 %p2"
+  "sub\\t%<w>0, %<w>4, %w1, <su>xt%e3 %p2"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -1916,7 +1916,7 @@
   "*
   operands[3] = GEN_INT (aarch64_uxt_size (exact_log2 (INTVAL (operands[2])),
 					   INTVAL (operands[3])));
-  return \"sub\t%<w>0, %<w>4, %<w>1, uxt%e3 %p2\";"
+  return \"sub\t%<w>0, %<w>4, %w1, uxt%e3 %p2\";"
   [(set_attr "type" "alu_ext")]
 )
 
@@ -2349,7 +2349,7 @@
 			 (match_operand:ALLX 0 "register_operand" "r"))
 			(match_operand:GPI 1 "register_operand" "r")))]
   ""
-  "cmp\\t%<GPI:w>1, %<GPI:w>0, <su>xt<ALLX:size>"
+  "cmp\\t%<GPI:w>1, %w0, <su>xt<ALLX:size>"
   [(set_attr "type" "alus_ext")]
 )
 
@@ -2361,7 +2361,7 @@
 			 (match_operand 1 "aarch64_imm3" "Ui3"))
 	(match_operand:GPI 2 "register_operand" "r")))]
   ""
-  "cmp\\t%<GPI:w>2, %<GPI:w>0, <su>xt<ALLX:size> %1"
+  "cmp\\t%<GPI:w>2, %w0, <su>xt<ALLX:size> %1"
   [(set_attr "type" "alus_ext")]
 )
 

[-- Attachment #3: patch2 --]
[-- Type: application/octet-stream, Size: 1187 bytes --]

Index: subs3.c
===================================================================
--- subs3.c	(revision 215364)
+++ subs3.c	(working copy)
@@ -58,4 +58,4 @@
   return 0;
 }
 
-/* { dg-final { scan-assembler-times "subs\tx\[0-9\]+, x\[0-9\]+, x\[0-9\]+, sxtw" 2 } } */
+/* { dg-final { scan-assembler-times "subs\tx\[0-9\]+, x\[0-9\]+, w\[0-9\]+, sxtw" 2 } } */
Index: adds3.c
===================================================================
--- adds3.c	(revision 215364)
+++ adds3.c	(working copy)
@@ -58,4 +58,4 @@
   return 0;
 }
 
-/* { dg-final { scan-assembler-times "adds\tx\[0-9\]+, x\[0-9\]+, x\[0-9\]+, sxtw" 2 } } */
+/* { dg-final { scan-assembler-times "adds\tx\[0-9\]+, x\[0-9\]+, w\[0-9\]+, sxtw" 2 } } */
Index: cmp.c
===================================================================
--- cmp.c	(revision 215364)
+++ cmp.c	(working copy)
@@ -58,4 +58,5 @@
 }
 
 /* { dg-final { scan-assembler-times "cmp\tw\[0-9\]+, w\[0-9\]+" 2 } } */
-/* { dg-final { scan-assembler-times "cmp\tx\[0-9\]+, x\[0-9\]+" 4 } } */
+/* { dg-final { scan-assembler-times "cmp\tx\[0-9\]+, x\[0-9\]+" 2 } } */
+/* { dg-final { scan-assembler-times "cmp\tx\[0-9\]+, w\[0-9\]+, sxtw" 2 } } */

Re: [Patch AArch64] Fix extended register width

[Carrot Wei]

Ping.

On Mon, Sep 22, 2014 at 11:41 AM, Carrot Wei <carrot@google.com> wrote:
> Hi
>
> The extended register width in add/adds/sub/subs/cmp instructions is
> not always the same as target register, it depends on both target
> register width and extension type. But in current implementation the
> extended register width is always the same as target register. We have
> noticed it can generate following wrong assembler code when compiled
> an internal application,
>
> add     x2, x20, x0, sxtw 3
>
> The correct assembler should be
>
> add     x2, x20, w0, sxtw 3
>
> On the other hand I noticed current gcc can only generate following
> extension types: xtb, xth, xtw. In these cases the extended register
> width can only be 'w'. So this patch changes the the extended register
> size attribute to 'w'.
>
> Passed regression tests on qemu without failure.
> OK for trunk and 4.9 branch?
>
> thanks
> Guozhi Wei
>
>
> 2014-09-22  Guozhi Wei  <carrot@google.com>
>
>         * config/aarch64/aarch64.md (*adds_<optab><ALLX:mode>_<GPI:mode>):
>         Change the extended register width to w.
>         (*subs_<optab><ALLX:mode>_<GPI:mode>): Likewise.
>         (*adds_<optab><mode>_multp2): Likewise.
>         (*subs_<optab><mode>_multp2): Likewise.
>         (*add_<optab><ALLX:mode>_<GPI:mode>): Likewise.
>         (*add_<optab><ALLX:mode>_shft_<GPI:mode>): Likewise.
>         (*add_<optab><ALLX:mode>_mult_<GPI:mode>): Likewise.
>         (*add_<optab><mode>_multp2): Likewise.
>         (*add_uxt<mode>_multp2): Likewise.
>         (*sub_<optab><ALLX:mode>_<GPI:mode>): Likewise.
>         (*sub_<optab><ALLX:mode>_shft_<GPI:mode>): Likewise.
>         (*sub_<optab><mode>_multp2): Likewise.
>         (*sub_uxt<mode>_multp2): Likewise.
>         (*cmp_swp_<optab><ALLX:mode>_reg<GPI:mode>): Likewise.
>         (*cmp_swp_<optab><ALLX:mode>_shft_<GPI:mode>): Likewise.
>
>
> 2014-09-22  Guozhi Wei  <carrot@google.com>
>
>         * gcc.target/aarch64/subs3.c: Change the extended register width to w.
>         * gcc.target/aarch64/adds3.c: Likewise.
>         * gcc.target/aarch64/cmp.c: Likewise.

Re: [Patch AArch64] Fix extended register width

[Marcus Shawcroft]

On 22 September 2014 19:41, Carrot Wei <carrot@google.com> wrote:
> Hi
>
> The extended register width in add/adds/sub/subs/cmp instructions is
> not always the same as target register, it depends on both target
> register width and extension type. But in current implementation the
> extended register width is always the same as target register. We have
> noticed it can generate following wrong assembler code when compiled
> an internal application,
>
> add     x2, x20, x0, sxtw 3
>
> The correct assembler should be
>
> add     x2, x20, w0, sxtw 3

Hi,

The assembler deliberately accepts the first form as a programmer
convenience.  Given the above example:

AARCH64 GAS  x.s page 1


   1 0000 82CE20AB        adds    x2, x20, x0, sxtw 3
   2 0004 82CE20AB        adds    x2, x20, w0, sxtw 3

Note both forms are correctly assembled.  The GAS implementation
contains code at (or near) tc-aarch64.c:5461 that specifically catches
the former.

... therefore I see no need to change the behaviour of gcc.

Cheers
/Marcus

Re: [Patch AArch64] Fix extended register width

[Eric Christopher]

On Tue, Sep 30, 2014 at 5:57 AM, Marcus Shawcroft
<marcus.shawcroft@gmail.com> wrote:
> On 22 September 2014 19:41, Carrot Wei <carrot@google.com> wrote:
>> Hi
>>
>> The extended register width in add/adds/sub/subs/cmp instructions is
>> not always the same as target register, it depends on both target
>> register width and extension type. But in current implementation the
>> extended register width is always the same as target register. We have
>> noticed it can generate following wrong assembler code when compiled
>> an internal application,
>>
>> add     x2, x20, x0, sxtw 3
>>
>> The correct assembler should be
>>
>> add     x2, x20, w0, sxtw 3
>

Hi Marcus,

> Hi,
>
> The assembler deliberately accepts the first form as a programmer
> convenience.  Given the above example:
>

I've been doing some reading of the ARM-v8 ARM and the language the
ARM uses here for this instruction matches the "shall" and not
"should" language it uses in other locations:

"Is the 32-bit name of the second general-purpose source register,
encoded in the "Rm" field."

This seems to say that a conforming assembler should error on a
non-32bit named register here. As I said, same sort of verbiage used
elsewhere for shall, in "should" cases the ARM is very careful to
spell it out.

Now if we want to change the ARM philosophy here I'm not opposed, but
I think we'd want some more explicit documentation about how/where
things should be more relaxed versus a bunch of "this is convenient to
accept here" stuff. That kind of thing has a tendency to end up in
some pretty fun context sensitive parsing madness.

Thoughts?

-eric


> AARCH64 GAS  x.s page 1
>
>
>    1 0000 82CE20AB        adds    x2, x20, x0, sxtw 3
>    2 0004 82CE20AB        adds    x2, x20, w0, sxtw 3
>
> Note both forms are correctly assembled.  The GAS implementation
> contains code at (or near) tc-aarch64.c:5461 that specifically catches
> the former.
>
> ... therefore I see no need to change the behaviour of gcc.
>
> Cheers
> /Marcus

Re: [Patch AArch64] Fix extended register width

[Richard Earnshaw]

On 30/09/14 21:30, Eric Christopher wrote:
> On Tue, Sep 30, 2014 at 5:57 AM, Marcus Shawcroft
> <marcus.shawcroft@gmail.com> wrote:
>> On 22 September 2014 19:41, Carrot Wei <carrot@google.com> wrote:
>>> Hi
>>>
>>> The extended register width in add/adds/sub/subs/cmp instructions is
>>> not always the same as target register, it depends on both target
>>> register width and extension type. But in current implementation the
>>> extended register width is always the same as target register. We have
>>> noticed it can generate following wrong assembler code when compiled
>>> an internal application,
>>>
>>> add     x2, x20, x0, sxtw 3
>>>
>>> The correct assembler should be
>>>
>>> add     x2, x20, w0, sxtw 3
>>
> 
> Hi Marcus,
> 
>> Hi,
>>
>> The assembler deliberately accepts the first form as a programmer
>> convenience.  Given the above example:
>>
> 
> I've been doing some reading of the ARM-v8 ARM and the language the
> ARM uses here for this instruction matches the "shall" and not
> "should" language it uses in other locations:
> 
> "Is the 32-bit name of the second general-purpose source register,
> encoded in the "Rm" field."
> 
> This seems to say that a conforming assembler should error on a
> non-32bit named register here. As I said, same sort of verbiage used
> elsewhere for shall, in "should" cases the ARM is very careful to
> spell it out.
> 
> Now if we want to change the ARM philosophy here I'm not opposed, but
> I think we'd want some more explicit documentation about how/where
> things should be more relaxed versus a bunch of "this is convenient to
> accept here" stuff. That kind of thing has a tendency to end up in
> some pretty fun context sensitive parsing madness.
> 

Agreed.  We're already working on it...

R.

Re: [Patch AArch64] Fix extended register width

[Xinliang David Li]

On Wed, Oct 1, 2014 at 1:42 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 30/09/14 21:30, Eric Christopher wrote:
>> On Tue, Sep 30, 2014 at 5:57 AM, Marcus Shawcroft
>> <marcus.shawcroft@gmail.com> wrote:
>>> On 22 September 2014 19:41, Carrot Wei <carrot@google.com> wrote:
>>>> Hi
>>>>
>>>> The extended register width in add/adds/sub/subs/cmp instructions is
>>>> not always the same as target register, it depends on both target
>>>> register width and extension type. But in current implementation the
>>>> extended register width is always the same as target register. We have
>>>> noticed it can generate following wrong assembler code when compiled
>>>> an internal application,
>>>>
>>>> add     x2, x20, x0, sxtw 3
>>>>
>>>> The correct assembler should be
>>>>
>>>> add     x2, x20, w0, sxtw 3
>>>
>>
>> Hi Marcus,
>>
>>> Hi,
>>>
>>> The assembler deliberately accepts the first form as a programmer
>>> convenience.  Given the above example:
>>>
>>
>> I've been doing some reading of the ARM-v8 ARM and the language the
>> ARM uses here for this instruction matches the "shall" and not
>> "should" language it uses in other locations:
>>
>> "Is the 32-bit name of the second general-purpose source register,
>> encoded in the "Rm" field."
>>
>> This seems to say that a conforming assembler should error on a
>> non-32bit named register here. As I said, same sort of verbiage used
>> elsewhere for shall, in "should" cases the ARM is very careful to
>> spell it out.
>>
>> Now if we want to change the ARM philosophy here I'm not opposed, but
>> I think we'd want some more explicit documentation about how/where
>> things should be more relaxed versus a bunch of "this is convenient to
>> accept here" stuff. That kind of thing has a tendency to end up in
>> some pretty fun context sensitive parsing madness.
>>
>
> Agreed.  We're already working on it...

I assume you mean 'working on the documentation' ?

David

>
> R.
>
>

Re: [Patch AArch64] Fix extended register width

[Eric Christopher]

On Wed, Oct 1, 2014 at 1:42 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 30/09/14 21:30, Eric Christopher wrote:
>> On Tue, Sep 30, 2014 at 5:57 AM, Marcus Shawcroft
>> <marcus.shawcroft@gmail.com> wrote:
>>> On 22 September 2014 19:41, Carrot Wei <carrot@google.com> wrote:
>>>> Hi
>>>>
>>>> The extended register width in add/adds/sub/subs/cmp instructions is
>>>> not always the same as target register, it depends on both target
>>>> register width and extension type. But in current implementation the
>>>> extended register width is always the same as target register. We have
>>>> noticed it can generate following wrong assembler code when compiled
>>>> an internal application,
>>>>
>>>> add     x2, x20, x0, sxtw 3
>>>>
>>>> The correct assembler should be
>>>>
>>>> add     x2, x20, w0, sxtw 3
>>>
>>
>> Hi Marcus,
>>
>>> Hi,
>>>
>>> The assembler deliberately accepts the first form as a programmer
>>> convenience.  Given the above example:
>>>
>>
>> I've been doing some reading of the ARM-v8 ARM and the language the
>> ARM uses here for this instruction matches the "shall" and not
>> "should" language it uses in other locations:
>>
>> "Is the 32-bit name of the second general-purpose source register,
>> encoded in the "Rm" field."
>>
>> This seems to say that a conforming assembler should error on a
>> non-32bit named register here. As I said, same sort of verbiage used
>> elsewhere for shall, in "should" cases the ARM is very careful to
>> spell it out.
>>
>> Now if we want to change the ARM philosophy here I'm not opposed, but
>> I think we'd want some more explicit documentation about how/where
>> things should be more relaxed versus a bunch of "this is convenient to
>> accept here" stuff. That kind of thing has a tendency to end up in
>> some pretty fun context sensitive parsing madness.
>>
>
> Agreed.  We're already working on it...
>

To be clear here I think the current language is exactly what it
should be in this case and that the explicit w register requirement is
a good thing for the assembly language.

-eric