> On 08.11.2018, at 13:25, Kyrill Tkachov <kyrylo.tkac...@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.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?
>
> 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.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
