Hi Kyrill,
> On 08.11.2018, at 11:16, Kyrill Tkachov <kyrylo.tkac...@foss.arm.com> wrote:
>
> Hi Christoph,
>
> On 07/11/18 21:51, christoph.muell...@theobroma-systems.com wrote:
>> From: Christoph Muellner <christoph.muell...@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.muell...@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.muell...@theobroma-systems.com>
>> Signed-off-by: Philipp Tomsich <philipp.toms...@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
