This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR61306: improve handling of sign and cast in bswap
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: "Thomas Preud'homme" <thomas dot preudhomme at arm dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 4 Jun 2014 11:39:08 +0200
- Subject: Re: [PATCH] Fix PR61306: improve handling of sign and cast in bswap
- Authentication-results: sourceware.org; auth=none
- References: <001d01cf7fba$259d63b0$70d82b10$ at arm dot com>
On Wed, Jun 4, 2014 at 7:59 AM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
> When bswap replace a bitwise expression involving a memory source by a load possibly followed by a bswap, it is possible that the load has a size smaller than that of the target expression where the bitwise expression was affected. So some sort of cast is needed. But there might also be a difference between the size of the expression that was affected and the size of the load. So 3 different sizes might be involved. Consider the following example from binutils:
>
> bfd_vma
> bfd_getl16 (const void *p)
> {
> const bfd_byte *addr = (*const bfd_byte *) p;
> return (addr[1] << 8) | addr[0];
> }
>
> Here the load will have a size of 16 bits, while the bitwise expression is an int (don't ask me why) but is returned as a 64 bits quantity (bfd_vma maps to the size of host registers). In this case we need 2 separate cast. One from 16 bit quantity to int with zero extension as the high bits are 0. It is always a zero extension because bswap will not do anything in the presence of a sign extension as depending on the initial value the result would be different (maybe a bswap if positive number and random value if negative number). Then, we need to cast respecting the extension that would have happen had we not replaced the bitwise extension. Here since the bitwise expression is int, it means we sign extend and then consider the content as being unsigned (bfd_vma is an unsigned quantity).
>
> When a bswap is necessary we need to do this double cast after doing the bswap as the bswap must be done on the loaded value since a that's the size expected by the bswap builtin. Finally, this patch also forbit any sign extension *in* the bitwise expression as the result of the expression would then be unpredictable (depend on the initial value).
>
> The patch works this way:
>
> 1) prevent size extension of a bitwise expression
> 2) record the type of the bitwise expression instead of its size (the size can be determined from the type)
> 3) use this type to perform a double cast as explained above
>
>
> 2014-05-30 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> PR tree-optimization/61306
> * tree-ssa-math-opts.c (struct symbolic_number): Store type of
> expression instead of its size.
> (do_shift_rotate): Adapt to change in struct symbolic_number.
> (verify_symbolic_number_p): Likewise.
> (init_symbolic_number): Likewise.
> (find_bswap_or_nop_1): Likewise. Also prevent optimization when the
> result of the expressions is unpredictable due to sign extension.
> (convert_via): New function to deal with the casting involved from the
> loaded value to the target SSA.
> (bswap_replace): Rename load_type in range_type to reflect it's the
> type the memory accessed shall have before being casted. Select load
> type according to whether a bswap needs to be done. Cast first to rhs
> with zero extend and then to lhs with sign extend to keep semantic of
> original stmts.
> (pass_optimize_bswap::execute): Adapt to change in struct
> symbolic_number. Decide if the range accessed should be signed or
> unsigned before being casted to lhs type based on rhs type and size.
>
> 2014-05-29 Thomas Preud'homme <thomas.preudhomme@arm.com>
>
> * gcc.c-torture/execute/pr61306.c: New test.
>
> Patch is in attachment. Is this ok for trunk?
I'd rather change the comparisons
- if (n->size < (int)sizeof (int64_t))
- n->n &= ((uint64_t)1 << (n->size * BITS_PER_UNIT)) - 1;
+ if (bitsize / BITS_PER_UNIT < (int)sizeof (int64_t))
+ n->n &= ((uint64_t)1 << bitsize) - 1;
to work in bits, thus bitsize < 8 * sizeof (int64_t) (note that using
BITS_PER_UNIT is bogus here - you are dealing with 8-bit bytes
on the host, not whatever the target uses). Otherwise it smells
like truncation may lose bits (probably not in practice).
+ /* Sign extension: result is dependent on the value. */
+ if (!TYPE_UNSIGNED (type) && !TYPE_UNSIGNED (n->type)
+ && type_size > TYPE_PRECISION (n->type))
+ return NULL_TREE;
whether it's sign-extended depends solely on the sign of the
converted entity, so I'm not sure why you are restricting this
to signed n->type. Consider
signed char *p;
((unsigned int)p[0]) << 8 | ((unsigned int)p[1]) << 16
the loads are still sign-extended but n->type is unsigned.
I'm failing to get why you possibly need two casts ... you should
only need one, from the bswap/load result to the final type
(zero-extended as you say - so the load type should simply be
unsigned which it is already).
So I think that the testcase in the patch is fixed already by
doing the n->type change (and a proper sign-extension detection).
Can you please split that part out?
That range_type and convert_via looks wrong and unnecessary to me,
and it doesn't look like you have a testcase excercising it?
Thanks,
Richard.
> Best regards,
>
> Thomas