On Wed, Jun 4, 2014 at 7:59 AM, Thomas Preud'homme
<thomas.preudho...@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.preudho...@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.preudho...@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
