[aarch64] Correct the maximum shift amount for shifted operands.

Message ID 20181107215123.16222-1-christoph.muellner@theobroma-systems.com
State New
Headers show
Series
  • [aarch64] Correct the maximum shift amount for shifted operands.
Related show

Commit Message

Christoph Muellner Nov. 7, 2018, 9:51 p.m.
From: Christoph Muellner <christoph.muellner@theobroma-systems.com>

The aarch64 ISA specification allows a left shift amount to be applied
after extension in the range of 0 to 4 (encoded in the imm3 field).

This is true for at least the following instructions:

 * ADD (extend register)
 * ADDS (extended register)
 * SUB (extended register)

The result of this patch can be seen, when compiling the following code:

uint64_t myadd(uint64_t a, uint64_t b)
{
    return a+(((uint8_t)b)<<4);
}

Without the patch the following sequence will be generated:

0000000000000000 <myadd>:
   0:	d37c1c21 	ubfiz	x1, x1, #4, #8
   4:	8b000020 	add	x0, x1, x0
   8:	d65f03c0 	ret

With the patch the ubfiz will be merged into the add instruction:

0000000000000000 <myadd>:
   0:	8b211000 	add	x0, x0, w1, uxtb #4
   4:	d65f03c0 	ret

*** gcc/ChangeLog ***

2018-xx-xx  Christoph Muellner <christoph.muellner@theobroma-system.com>

	* gcc/config/aarch64/aarch64.c: Correct the maximum shift amount
	for shifted operands.
	* gcc/testsuite/gcc.target/aarch64/extend.c: Adjust the
	testcases to cover the changed shift amount.

Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
---
 gcc/config/aarch64/aarch64.c              |  2 +-
 gcc/testsuite/gcc.target/aarch64/extend.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Kyrill Tkachov Nov. 8, 2018, 10:16 a.m. | #1
Hi Christoph,

On 07/11/18 21:51, christoph.muellner@theobroma-systems.com wrote:
> From: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>
> The aarch64 ISA specification allows a left shift amount to be applied
> after extension in the range of 0 to 4 (encoded in the imm3 field).
>

Indeed. That looks correct from my reading of the spec as well (section C6.2.3 for example).

> This is true for at least the following instructions:
>
>  * ADD (extend register)
>  * ADDS (extended register)
>  * SUB (extended register)
>
> The result of this patch can be seen, when compiling the following code:
>
> uint64_t myadd(uint64_t a, uint64_t b)
> {
>     return a+(((uint8_t)b)<<4);
> }
>
> Without the patch the following sequence will be generated:
>
> 0000000000000000 <myadd>:
>    0:   d37c1c21         ubfiz   x1, x1, #4, #8
>    4:   8b000020         add     x0, x1, x0
>    8:   d65f03c0         ret
>
> With the patch the ubfiz will be merged into the add instruction:
>
> 0000000000000000 <myadd>:
>    0:   8b211000         add     x0, x0, w1, uxtb #4
>    4:   d65f03c0         ret
>
> *** gcc/ChangeLog ***
>

The patch looks correct to me but can you please clarify how it has been tested?
Patches need to be bootstrapped and the full testsuite run.

I also have a few comments on the ChangeLog

> 2018-xx-xx  Christoph Muellner <christoph.muellner@theobroma-system.com>
>

Two spaces between your name and the email (also, missing an 's' in the email?).

>         * gcc/config/aarch64/aarch64.c: Correct the maximum shift amount
>         for shifted operands.

The path should be relative to the ChangeLog location.
Also, please specify the function name being changed.
In this case it should be
     * config/aarch64/aarch64.c (aarch64_uxt_size): Correct...

>         * gcc/testsuite/gcc.target/aarch64/extend.c: Adjust the
>         testcases to cover the changed shift amount.

This should be in a separate ChangeLog entry as it will go into gcc/testsuite/ChangeLog.
The path would be:
     * gcc.target/aarch64/extend.c

>
> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>

GCC doesn't use Signed-off-by tags (though no one will complain about them either) but if
Philipp Tomsich wrote any of the code here (which I think is implied by the Signed-off-by tag?) then
his name should appear in the ChangeLog entry.

Thanks for the patch!
As I said, the code looks good to me, but you'll need a maintainer to approve it
(I've cc'ed the aarch64 maintainers for you) once the testing is clarified and the ChangeLog is fixed.

Kyrill


> ---
>  gcc/config/aarch64/aarch64.c              |  2 +-
>  gcc/testsuite/gcc.target/aarch64/extend.c | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index c82c7b6..c85988a 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -8190,7 +8190,7 @@ aarch64_output_casesi (rtx *operands)
>  int
>  aarch64_uxt_size (int shift, HOST_WIDE_INT mask)
>  {
> -  if (shift >= 0 && shift <= 3)
> +  if (shift >= 0 && shift <= 4)
>      {
>        int size;
>        for (size = 8; size <= 32; size *= 2)
> diff --git a/gcc/testsuite/gcc.target/aarch64/extend.c b/gcc/testsuite/gcc.target/aarch64/extend.c
> index f399e55..7986c5b 100644
> --- a/gcc/testsuite/gcc.target/aarch64/extend.c
> +++ b/gcc/testsuite/gcc.target/aarch64/extend.c
> @@ -32,8 +32,8 @@ ldr_sxtw0 (char *arr, int i)
>  unsigned long long
>  adddi_uxtw (unsigned long long a, unsigned int i)
>  {
> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?3" } } */
> -  return a + ((unsigned long long)i << 3);
> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?4" } } */
> +  return a + ((unsigned long long)i << 4);
>  }
>
>  unsigned long long
> @@ -46,8 +46,8 @@ adddi_uxtw0 (unsigned long long a, unsigned int i)
>  long long
>  adddi_sxtw (long long a, int i)
>  {
> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?3" } } */
> -  return a + ((long long)i << 3);
> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?4" } } */
> +  return a + ((long long)i << 4);
>  }
>
>  long long
> @@ -60,8 +60,8 @@ adddi_sxtw0 (long long a, int i)
>  unsigned long long
>  subdi_uxtw (unsigned long long a, unsigned int i)
>  {
> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?3" } } */
> -  return a - ((unsigned long long)i << 3);
> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?4" } } */
> +  return a - ((unsigned long long)i << 4);
>  }
>
>  unsigned long long
> @@ -74,8 +74,8 @@ subdi_uxtw0 (unsigned long long a, unsigned int i)
>  long long
>  subdi_sxtw (long long a, int i)
>  {
> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?3" } } */
> -  return a - ((long long)i << 3);
> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?4" } } */
> +  return a - ((long long)i << 4);
>  }
>
>  long long
> -- 
> 2.9.5
>
Christoph Muellner Nov. 8, 2018, 12:20 p.m. | #2
Hi Kyrill,

> On 08.11.2018, at 11:16, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
> Hi Christoph,
> 
> On 07/11/18 21:51, christoph.muellner@theobroma-systems.com wrote:
>> From: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> 
>> The aarch64 ISA specification allows a left shift amount to be applied
>> after extension in the range of 0 to 4 (encoded in the imm3 field).
>> 
> 
> Indeed. That looks correct from my reading of the spec as well (section C6.2.3 for example).
> 
>> This is true for at least the following instructions:
>> 
>> * ADD (extend register)
>> * ADDS (extended register)
>> * SUB (extended register)
>> 
>> The result of this patch can be seen, when compiling the following code:
>> 
>> uint64_t myadd(uint64_t a, uint64_t b)
>> {
>>    return a+(((uint8_t)b)<<4);
>> }
>> 
>> Without the patch the following sequence will be generated:
>> 
>> 0000000000000000 <myadd>:
>>   0:   d37c1c21         ubfiz   x1, x1, #4, #8
>>   4:   8b000020         add     x0, x1, x0
>>   8:   d65f03c0         ret
>> 
>> With the patch the ubfiz will be merged into the add instruction:
>> 
>> 0000000000000000 <myadd>:
>>   0:   8b211000         add     x0, x0, w1, uxtb #4
>>   4:   d65f03c0         ret
>> 
>> *** gcc/ChangeLog ***
>> 
> 
> The patch looks correct to me but can you please clarify how it has been tested?
> Patches need to be bootstrapped and the full testsuite run.

I've tested native (on an aarch64 system) with
> make bootstrap
> make check RUNTESTFLAGS="aarch64.exp"
and haven't found any regressions (compared to this patch not being applied).

Currently I'm running make check without any RUNTESTFLAGS, but that will take
some time to finish (already 1.5 hours running on a 3 GHz machine).

My approach to evaluate the test run is to diff the output of make check
(with and without the patch applied). Is there a better approach for this?
Especially is there a way to parallelise test execution?
I have 32 cores, which could significantly reduce the test duration,
but when using -j32, then my diff approach does not work anymore.
How is this intended to be done?

> I also have a few comments on the ChangeLog
> 
>> 2018-xx-xx  Christoph Muellner <christoph.muellner@theobroma-system.com>
>> 
> 
> Two spaces between your name and the email (also, missing an 's' in the email?).
> 
>>        * gcc/config/aarch64/aarch64.c: Correct the maximum shift amount
>>        for shifted operands.
> 
> The path should be relative to the ChangeLog location.
> Also, please specify the function name being changed.
> In this case it should be
>    * config/aarch64/aarch64.c (aarch64_uxt_size): Correct...
> 
>>        * gcc/testsuite/gcc.target/aarch64/extend.c: Adjust the
>>        testcases to cover the changed shift amount.
> 
> This should be in a separate ChangeLog entry as it will go into gcc/testsuite/ChangeLog.
> The path would be:
>    * gcc.target/aarch64/extend.c
> 
>> 
>> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> 
> GCC doesn't use Signed-off-by tags (though no one will complain about them either) but if
> Philipp Tomsich wrote any of the code here (which I think is implied by the Signed-off-by tag?) then
> his name should appear in the ChangeLog entry.

Thank's for your comments, I've already addressed them locally.
I'll resend once testing is completed.

Thanks,
Christoph

> Thanks for the patch!
> As I said, the code looks good to me, but you'll need a maintainer to approve it
> (I've cc'ed the aarch64 maintainers for you) once the testing is clarified and the ChangeLog is fixed.
> 
> Kyrill
> 
> 
>> ---
>> gcc/config/aarch64/aarch64.c              |  2 +-
>> gcc/testsuite/gcc.target/aarch64/extend.c | 16 ++++++++--------
>> 2 files changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index c82c7b6..c85988a 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -8190,7 +8190,7 @@ aarch64_output_casesi (rtx *operands)
>> int
>> aarch64_uxt_size (int shift, HOST_WIDE_INT mask)
>> {
>> -  if (shift >= 0 && shift <= 3)
>> +  if (shift >= 0 && shift <= 4)
>>     {
>>       int size;
>>       for (size = 8; size <= 32; size *= 2)
>> diff --git a/gcc/testsuite/gcc.target/aarch64/extend.c b/gcc/testsuite/gcc.target/aarch64/extend.c
>> index f399e55..7986c5b 100644
>> --- a/gcc/testsuite/gcc.target/aarch64/extend.c
>> +++ b/gcc/testsuite/gcc.target/aarch64/extend.c
>> @@ -32,8 +32,8 @@ ldr_sxtw0 (char *arr, int i)
>> unsigned long long
>> adddi_uxtw (unsigned long long a, unsigned int i)
>> {
>> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?3" } } */
>> -  return a + ((unsigned long long)i << 3);
>> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?4" } } */
>> +  return a + ((unsigned long long)i << 4);
>> }
>> 
>> unsigned long long
>> @@ -46,8 +46,8 @@ adddi_uxtw0 (unsigned long long a, unsigned int i)
>> long long
>> adddi_sxtw (long long a, int i)
>> {
>> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?3" } } */
>> -  return a + ((long long)i << 3);
>> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?4" } } */
>> +  return a + ((long long)i << 4);
>> }
>> 
>> long long
>> @@ -60,8 +60,8 @@ adddi_sxtw0 (long long a, int i)
>> unsigned long long
>> subdi_uxtw (unsigned long long a, unsigned int i)
>> {
>> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?3" } } */
>> -  return a - ((unsigned long long)i << 3);
>> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?4" } } */
>> +  return a - ((unsigned long long)i << 4);
>> }
>> 
>> unsigned long long
>> @@ -74,8 +74,8 @@ subdi_uxtw0 (unsigned long long a, unsigned int i)
>> long long
>> subdi_sxtw (long long a, int i)
>> {
>> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?3" } } */
>> -  return a - ((long long)i << 3);
>> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?4" } } */
>> +  return a - ((long long)i << 4);
>> }
>> 
>> long long
>> -- 
>> 2.9.5
Kyrill Tkachov Nov. 8, 2018, 12:25 p.m. | #3
Hi Christoph,

On 08/11/18 12:20, Christoph Müllner wrote:
> Hi Kyrill,
>
> > On 08.11.2018, at 11:16, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> >
> > Hi Christoph,
> >
> > On 07/11/18 21:51, christoph.muellner@theobroma-systems.com wrote:
> >> From: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> >>
> >> The aarch64 ISA specification allows a left shift amount to be applied
> >> after extension in the range of 0 to 4 (encoded in the imm3 field).
> >>
> >
> > Indeed. That looks correct from my reading of the spec as well (section C6.2.3 for example).
> >
> >> This is true for at least the following instructions:
> >>
> >> * ADD (extend register)
> >> * ADDS (extended register)
> >> * SUB (extended register)
> >>
> >> The result of this patch can be seen, when compiling the following code:
> >>
> >> uint64_t myadd(uint64_t a, uint64_t b)
> >> {
> >>    return a+(((uint8_t)b)<<4);
> >> }
> >>
> >> Without the patch the following sequence will be generated:
> >>
> >> 0000000000000000 <myadd>:
> >>   0:   d37c1c21         ubfiz   x1, x1, #4, #8
> >>   4:   8b000020         add     x0, x1, x0
> >>   8:   d65f03c0         ret
> >>
> >> With the patch the ubfiz will be merged into the add instruction:
> >>
> >> 0000000000000000 <myadd>:
> >>   0:   8b211000         add     x0, x0, w1, uxtb #4
> >>   4:   d65f03c0         ret
> >>
> >> *** gcc/ChangeLog ***
> >>
> >
> > The patch looks correct to me but can you please clarify how it has been tested?
> > Patches need to be bootstrapped and the full testsuite run.
>
> I've tested native (on an aarch64 system) with
> > make bootstrap
> > make check RUNTESTFLAGS="aarch64.exp"
> and haven't found any regressions (compared to this patch not being applied).
>
> Currently I'm running make check without any RUNTESTFLAGS, but that will take
> some time to finish (already 1.5 hours running on a 3 GHz machine).
>
> My approach to evaluate the test run is to diff the output of make check
> (with and without the patch applied). Is there a better approach for this?
> Especially is there a way to parallelise test execution?
> I have 32 cores, which could significantly reduce the test duration,
> but when using -j32, then my diff approach does not work anymore.
> How is this intended to be done?

Feel free to add -j32, that will make testing much more practicable.
The way to check the results is to look at the .sum and .log files and compare them.

For example, in the build directory in $BUILD/gcc/testsuite

$ find. -name *.sum

./gfortran/gfortran.sum
./gcc/gcc.sum
./g++/g++.sum

You can compare these before and after your patch. These are generated in a consistent format, even with a -j make option.

You can also use the scripts in contrib/ (dg-extract-results.sh and dg-cmp-results.sh) in the source tree that do some
of the log extracting and analysis for you.

Thanks,
Kyrill

>
> > I also have a few comments on the ChangeLog
> >
> >> 2018-xx-xx  Christoph Muellner <christoph.muellner@theobroma-system.com>
> >>
> >
> > Two spaces between your name and the email (also, missing an 's' in the email?).
> >
> >>        * gcc/config/aarch64/aarch64.c: Correct the maximum shift amount
> >>        for shifted operands.
> >
> > The path should be relative to the ChangeLog location.
> > Also, please specify the function name being changed.
> > In this case it should be
> >    * config/aarch64/aarch64.c (aarch64_uxt_size): Correct...
> >
> >>        * gcc/testsuite/gcc.target/aarch64/extend.c: Adjust the
> >>        testcases to cover the changed shift amount.
> >
> > This should be in a separate ChangeLog entry as it will go into gcc/testsuite/ChangeLog.
> > The path would be:
> >    * gcc.target/aarch64/extend.c
> >
> >>
> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> >
> > GCC doesn't use Signed-off-by tags (though no one will complain about them either) but if
> > Philipp Tomsich wrote any of the code here (which I think is implied by the Signed-off-by tag?) then
> > his name should appear in the ChangeLog entry.
>
> Thank's for your comments, I've already addressed them locally.
> I'll resend once testing is completed.
>
> Thanks,
> Christoph
>
> > Thanks for the patch!
> > As I said, the code looks good to me, but you'll need a maintainer to approve it
> > (I've cc'ed the aarch64 maintainers for you) once the testing is clarified and the ChangeLog is fixed.
> >
> > Kyrill
> >
> >
> >> ---
> >> gcc/config/aarch64/aarch64.c              |  2 +-
> >> gcc/testsuite/gcc.target/aarch64/extend.c | 16 ++++++++--------
> >> 2 files changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> index c82c7b6..c85988a 100644
> >> --- a/gcc/config/aarch64/aarch64.c
> >> +++ b/gcc/config/aarch64/aarch64.c
> >> @@ -8190,7 +8190,7 @@ aarch64_output_casesi (rtx *operands)
> >> int
> >> aarch64_uxt_size (int shift, HOST_WIDE_INT mask)
> >> {
> >> -  if (shift >= 0 && shift <= 3)
> >> +  if (shift >= 0 && shift <= 4)
> >>     {
> >>       int size;
> >>       for (size = 8; size <= 32; size *= 2)
> >> diff --git a/gcc/testsuite/gcc.target/aarch64/extend.c b/gcc/testsuite/gcc.target/aarch64/extend.c
> >> index f399e55..7986c5b 100644
> >> --- a/gcc/testsuite/gcc.target/aarch64/extend.c
> >> +++ b/gcc/testsuite/gcc.target/aarch64/extend.c
> >> @@ -32,8 +32,8 @@ ldr_sxtw0 (char *arr, int i)
> >> unsigned long long
> >> adddi_uxtw (unsigned long long a, unsigned int i)
> >> {
> >> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?3" } } */
> >> -  return a + ((unsigned long long)i << 3);
> >> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?4" } } */
> >> +  return a + ((unsigned long long)i << 4);
> >> }
> >>
> >> unsigned long long
> >> @@ -46,8 +46,8 @@ adddi_uxtw0 (unsigned long long a, unsigned int i)
> >> long long
> >> adddi_sxtw (long long a, int i)
> >> {
> >> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?3" } } */
> >> -  return a + ((long long)i << 3);
> >> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?4" } } */
> >> +  return a + ((long long)i << 4);
> >> }
> >>
> >> long long
> >> @@ -60,8 +60,8 @@ adddi_sxtw0 (long long a, int i)
> >> unsigned long long
> >> subdi_uxtw (unsigned long long a, unsigned int i)
> >> {
> >> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?3" } } */
> >> -  return a - ((unsigned long long)i << 3);
> >> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?4" } } */
> >> +  return a - ((unsigned long long)i << 4);
> >> }
> >>
> >> unsigned long long
> >> @@ -74,8 +74,8 @@ subdi_uxtw0 (unsigned long long a, unsigned int i)
> >> long long
> >> subdi_sxtw (long long a, int i)
> >> {
> >> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?3" } } */
> >> -  return a - ((long long)i << 3);
> >> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?4" } } */
> >> +  return a - ((long long)i << 4);
> >> }
> >>
> >> long long
> >> --
> >> 2.9.5
>
Christoph Muellner Nov. 8, 2018, 12:28 p.m. | #4
> On 08.11.2018, at 13:25, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
> 
> Hi Christoph,
> 
> On 08/11/18 12:20, Christoph Müllner wrote:
>> Hi Kyrill,
>> 
>> > On 08.11.2018, at 11:16, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>> >
>> > Hi Christoph,
>> >
>> > On 07/11/18 21:51, christoph.muellner@theobroma-systems.com wrote:
>> >> From: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> >>
>> >> The aarch64 ISA specification allows a left shift amount to be applied
>> >> after extension in the range of 0 to 4 (encoded in the imm3 field).
>> >>
>> >
>> > Indeed. That looks correct from my reading of the spec as well (section C6.2.3 for example).
>> >
>> >> This is true for at least the following instructions:
>> >>
>> >> * ADD (extend register)
>> >> * ADDS (extended register)
>> >> * SUB (extended register)
>> >>
>> >> The result of this patch can be seen, when compiling the following code:
>> >>
>> >> uint64_t myadd(uint64_t a, uint64_t b)
>> >> {
>> >>    return a+(((uint8_t)b)<<4);
>> >> }
>> >>
>> >> Without the patch the following sequence will be generated:
>> >>
>> >> 0000000000000000 <myadd>:
>> >>   0:   d37c1c21         ubfiz   x1, x1, #4, #8
>> >>   4:   8b000020         add     x0, x1, x0
>> >>   8:   d65f03c0         ret
>> >>
>> >> With the patch the ubfiz will be merged into the add instruction:
>> >>
>> >> 0000000000000000 <myadd>:
>> >>   0:   8b211000         add     x0, x0, w1, uxtb #4
>> >>   4:   d65f03c0         ret
>> >>
>> >> *** gcc/ChangeLog ***
>> >>
>> >
>> > The patch looks correct to me but can you please clarify how it has been tested?
>> > Patches need to be bootstrapped and the full testsuite run.
>> 
>> I've tested native (on an aarch64 system) with
>> > make bootstrap
>> > make check RUNTESTFLAGS="aarch64.exp"
>> and haven't found any regressions (compared to this patch not being applied).
>> 
>> Currently I'm running make check without any RUNTESTFLAGS, but that will take
>> some time to finish (already 1.5 hours running on a 3 GHz machine).
>> 
>> My approach to evaluate the test run is to diff the output of make check
>> (with and without the patch applied). Is there a better approach for this?
>> Especially is there a way to parallelise test execution?
>> I have 32 cores, which could significantly reduce the test duration,
>> but when using -j32, then my diff approach does not work anymore.
>> How is this intended to be done?
> 
> Feel free to add -j32, that will make testing much more practicable.
> The way to check the results is to look at the .sum and .log files and compare them.
> 
> For example, in the build directory in $BUILD/gcc/testsuite
> 
> $ find. -name *.sum
> 
> ./gfortran/gfortran.sum
> ./gcc/gcc.sum
> ./g++/g++.sum
> 
> You can compare these before and after your patch. These are generated in a consistent format, even with a -j make option.
> 
> You can also use the scripts in contrib/ (dg-extract-results.sh and dg-cmp-results.sh) in the source tree that do some
> of the log extracting and analysis for you.

Exactly what I was searching for!

Thanks,
Christoph

> 
> Thanks,
> Kyrill
> 
>> 
>> > I also have a few comments on the ChangeLog
>> >
>> >> 2018-xx-xx  Christoph Muellner <christoph.muellner@theobroma-system.com>
>> >>
>> >
>> > Two spaces between your name and the email (also, missing an 's' in the email?).
>> >
>> >>        * gcc/config/aarch64/aarch64.c: Correct the maximum shift amount
>> >>        for shifted operands.
>> >
>> > The path should be relative to the ChangeLog location.
>> > Also, please specify the function name being changed.
>> > In this case it should be
>> >    * config/aarch64/aarch64.c (aarch64_uxt_size): Correct...
>> >
>> >>        * gcc/testsuite/gcc.target/aarch64/extend.c: Adjust the
>> >>        testcases to cover the changed shift amount.
>> >
>> > This should be in a separate ChangeLog entry as it will go into gcc/testsuite/ChangeLog.
>> > The path would be:
>> >    * gcc.target/aarch64/extend.c
>> >
>> >>
>> >> Signed-off-by: Christoph Muellner <christoph.muellner@theobroma-systems.com>
>> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> >
>> > GCC doesn't use Signed-off-by tags (though no one will complain about them either) but if
>> > Philipp Tomsich wrote any of the code here (which I think is implied by the Signed-off-by tag?) then
>> > his name should appear in the ChangeLog entry.
>> 
>> Thank's for your comments, I've already addressed them locally.
>> I'll resend once testing is completed.
>> 
>> Thanks,
>> Christoph
>> 
>> > Thanks for the patch!
>> > As I said, the code looks good to me, but you'll need a maintainer to approve it
>> > (I've cc'ed the aarch64 maintainers for you) once the testing is clarified and the ChangeLog is fixed.
>> >
>> > Kyrill
>> >
>> >
>> >> ---
>> >> gcc/config/aarch64/aarch64.c              |  2 +-
>> >> gcc/testsuite/gcc.target/aarch64/extend.c | 16 ++++++++--------
>> >> 2 files changed, 9 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> >> index c82c7b6..c85988a 100644
>> >> --- a/gcc/config/aarch64/aarch64.c
>> >> +++ b/gcc/config/aarch64/aarch64.c
>> >> @@ -8190,7 +8190,7 @@ aarch64_output_casesi (rtx *operands)
>> >> int
>> >> aarch64_uxt_size (int shift, HOST_WIDE_INT mask)
>> >> {
>> >> -  if (shift >= 0 && shift <= 3)
>> >> +  if (shift >= 0 && shift <= 4)
>> >>     {
>> >>       int size;
>> >>       for (size = 8; size <= 32; size *= 2)
>> >> diff --git a/gcc/testsuite/gcc.target/aarch64/extend.c b/gcc/testsuite/gcc.target/aarch64/extend.c
>> >> index f399e55..7986c5b 100644
>> >> --- a/gcc/testsuite/gcc.target/aarch64/extend.c
>> >> +++ b/gcc/testsuite/gcc.target/aarch64/extend.c
>> >> @@ -32,8 +32,8 @@ ldr_sxtw0 (char *arr, int i)
>> >> unsigned long long
>> >> adddi_uxtw (unsigned long long a, unsigned int i)
>> >> {
>> >> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?3" } } */
>> >> -  return a + ((unsigned long long)i << 3);
>> >> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?4" } } */
>> >> +  return a + ((unsigned long long)i << 4);
>> >> }
>> >>
>> >> unsigned long long
>> >> @@ -46,8 +46,8 @@ adddi_uxtw0 (unsigned long long a, unsigned int i)
>> >> long long
>> >> adddi_sxtw (long long a, int i)
>> >> {
>> >> -  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?3" } } */
>> >> -  return a + ((long long)i << 3);
>> >> +  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?4" } } */
>> >> +  return a + ((long long)i << 4);
>> >> }
>> >>
>> >> long long
>> >> @@ -60,8 +60,8 @@ adddi_sxtw0 (long long a, int i)
>> >> unsigned long long
>> >> subdi_uxtw (unsigned long long a, unsigned int i)
>> >> {
>> >> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?3" } } */
>> >> -  return a - ((unsigned long long)i << 3);
>> >> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?4" } } */
>> >> +  return a - ((unsigned long long)i << 4);
>> >> }
>> >>
>> >> unsigned long long
>> >> @@ -74,8 +74,8 @@ subdi_uxtw0 (unsigned long long a, unsigned int i)
>> >> long long
>> >> subdi_sxtw (long long a, int i)
>> >> {
>> >> -  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?3" } } */
>> >> -  return a - ((long long)i << 3);
>> >> +  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?4" } } */
>> >> +  return a - ((long long)i << 4);
>> >> }
>> >>
>> >> long long
>> >> --
>> >> 2.9.5

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index c82c7b6..c85988a 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -8190,7 +8190,7 @@  aarch64_output_casesi (rtx *operands)
 int
 aarch64_uxt_size (int shift, HOST_WIDE_INT mask)
 {
-  if (shift >= 0 && shift <= 3)
+  if (shift >= 0 && shift <= 4)
     {
       int size;
       for (size = 8; size <= 32; size *= 2)
diff --git a/gcc/testsuite/gcc.target/aarch64/extend.c b/gcc/testsuite/gcc.target/aarch64/extend.c
index f399e55..7986c5b 100644
--- a/gcc/testsuite/gcc.target/aarch64/extend.c
+++ b/gcc/testsuite/gcc.target/aarch64/extend.c
@@ -32,8 +32,8 @@  ldr_sxtw0 (char *arr, int i)
 unsigned long long
 adddi_uxtw (unsigned long long a, unsigned int i)
 {
-  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?3" } } */
-  return a + ((unsigned long long)i << 3);
+  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*uxtw #?4" } } */
+  return a + ((unsigned long long)i << 4);
 }
 
 unsigned long long
@@ -46,8 +46,8 @@  adddi_uxtw0 (unsigned long long a, unsigned int i)
 long long
 adddi_sxtw (long long a, int i)
 {
-  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?3" } } */
-  return a + ((long long)i << 3);
+  /* { dg-final { scan-assembler "add\tx\[0-9\]+,.*sxtw #?4" } } */
+  return a + ((long long)i << 4);
 }
 
 long long
@@ -60,8 +60,8 @@  adddi_sxtw0 (long long a, int i)
 unsigned long long
 subdi_uxtw (unsigned long long a, unsigned int i)
 {
-  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?3" } } */
-  return a - ((unsigned long long)i << 3);
+  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*uxtw #?4" } } */
+  return a - ((unsigned long long)i << 4);
 }
 
 unsigned long long
@@ -74,8 +74,8 @@  subdi_uxtw0 (unsigned long long a, unsigned int i)
 long long
 subdi_sxtw (long long a, int i)
 {
-  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?3" } } */
-  return a - ((long long)i << 3);
+  /* { dg-final { scan-assembler "sub\tx\[0-9\]+,.*sxtw #?4" } } */
+  return a - ((long long)i << 4);
 }
 
 long long