Hi Thomas,
Some minor comments:
On 21 October 2014 11:28, Thomas Preud'homme <thomas.preudho...@arm.com> wrote:
> Hi Richard,
>
> I realized thanks to Christophe Lyon that a shift was not right: the shift
> count
> is a number of bytes instead of a number of bits.
>
> This extra patch fixes the problem.
>
> ChangeLog are as follows:
>
> *** gcc/ChangeLog ***
>
> 2014-09-26 Thomas Preud'homme <thomas.preudho...@arm.com>
>
> * tree-ssa-math-opts.c (find_bswap_or_nop_1): Fix creation of
> MARKER_BYTE_UNKNOWN markers when handling casts.
>
> *** gcc/testsuite/ChangeLog ***
>
> 2014-10-08 Thomas Preud'homme <thomas.preudho...@arm.com>
>
> * gcc.dg/optimize-bswaphi-1.c: New bswap pass test.
>
> diff --git a/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
> b/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
> index 3e51f04..18aba28 100644
> --- a/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
> +++ b/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
> @@ -42,6 +42,20 @@ uint32_t read_be16_3 (unsigned char *data)
> return *(data + 1) | (*data << 8);
> }
>
> +typedef int SItype __attribute__ ((mode (SI)));
What's the purpose of this? It seems unused.
> +typedef int HItype __attribute__ ((mode (HI)));
> +
> +/* Test that detection of significant sign extension works correctly. This
> + checks that unknown byte marker are set correctly in cast of cast. */
I believe this should be:
"checks that unknown byte markers are set correctly in case of cast"
> +
> +HItype
> +swap16 (HItype in)
> +{
> + return (HItype) (((in >> 0) & 0xFF) << 8)
> + | (((in >> 8) & 0xFF) << 0);
> +}
> +
> /* { dg-final { scan-tree-dump-times "16 bit load in target endianness found
> at" 3 "bswap" } } */
> -/* { dg-final { scan-tree-dump-times "16 bit bswap implementation found at"
> 3 "bswap" { xfail alpha*-*-* arm*-*-* } } } */
> +/* { dg-final { scan-tree-dump-times "16 bit bswap implementation found at"
> 1 "bswap" { target alpha*-*-* arm*-*-* } } } */
This line fails when forcing the compiler to target -march=armv5t for
instance. I suspect this is because the check_effective_target_bswap
test is too permissive.
Christophe.
> +/* { dg-final { scan-tree-dump-times "16 bit bswap implementation found at"
> 4 "bswap" { xfail alpha*-*-* arm*-*-* } } } */
> /* { dg-final { cleanup-tree-dump "bswap" } } */
> diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
> index 3c6e935..2ef2333 100644
> --- a/gcc/tree-ssa-math-opts.c
> +++ b/gcc/tree-ssa-math-opts.c
> @@ -1916,7 +1916,8 @@ find_bswap_or_nop_1 (gimple stmt, struct
> symbolic_number *n, int limit)
> if (!TYPE_UNSIGNED (n->type) && type_size > old_type_size
> && HEAD_MARKER (n->n, old_type_size))
> for (i = 0; i < type_size - old_type_size; i++)
> - n->n |= MARKER_BYTE_UNKNOWN << (type_size - 1 - i);
> + n->n |= MARKER_BYTE_UNKNOWN
> + << ((type_size - 1 - i) * BITS_PER_MARKER);
>
> if (type_size < 64 / BITS_PER_MARKER)
> {
>
> regression testsuite run without regression on x86_64-linux-gnu and bswap
> tests all pass on arm-none-eabi target
>
> Is it ok for trunk?
>
> Best regards,
>
> Thomas
>
>> -----Original Message-----
>> From: Richard Biener [mailto:richard.guent...@gmail.com]
>> Sent: Wednesday, September 24, 2014 4:01 PM
>> To: Thomas Preud'homme
>> Cc: GCC Patches
>> Subject: Re: [PATCH] Fix PR63266: Keep track of impact of sign extension
>> in bswap
>>
>> On Tue, Sep 16, 2014 at 12:24 PM, Thomas Preud'homme
>> <thomas.preudho...@arm.com> wrote:
>> > Hi all,
>> >
>> > The fix for PR61306 disabled bswap when a sign extension is detected.
>> However this led to a test case regression (and potential performance
>> regression) in case where a sign extension happens but its effect is
>> canceled by other bit manipulation. This patch aims to fix that by having a
>> special marker to track bytes whose value is unpredictable due to sign
>> extension. If the final result of a bit manipulation doesn't contain any
>> such marker then the bswap optimization can proceed.
>>
>> Nice and simple idea.
>>
>> Ok.
>>
>> Thanks,
>> Richard.
>>
>> > *** gcc/ChangeLog ***
>> >
>> > 2014-09-15 Thomas Preud'homme <thomas.preudho...@arm.com>
>> >
>> > PR tree-optimization/63266
>> > * tree-ssa-math-opts.c (struct symbolic_number): Add comment
>> about
>> > marker for unknown byte value.
>> > (MARKER_MASK): New macro.
>> > (MARKER_BYTE_UNKNOWN): New macro.
>> > (HEAD_MARKER): New macro.
>> > (do_shift_rotate): Mark bytes with unknown values due to sign
>> > extension when doing an arithmetic right shift. Replace hardcoded
>> > mask for marker by new MARKER_MASK macro.
>> > (find_bswap_or_nop_1): Likewise and adjust ORing of two
>> symbolic
>> > numbers accordingly.
>> >
>> > *** gcc/testsuite/ChangeLog ***
>> >
>> > 2014-09-15 Thomas Preud'homme <thomas.preudho...@arm.com>
>> >
>> > PR tree-optimization/63266
>> > * gcc.dg/optimize-bswapsi-1.c (swap32_d): New bswap pass test.
>> >
>> >
>> > Testing:
>> >
>> > * Built an arm-none-eabi-gcc cross-compiler and used it to run the
>> testsuite on QEMU emulating Cortex-M3 without any regression
>> > * Bootstrapped on x86_64-linux-gnu target and testsuite was run
>> without regression
>> >
>> >
>> > Ok for trunk?
>
>
>
>
