On Wed, Jun 4, 2014 at 12:42 PM, Thomas Preud'homme <thomas.preudho...@arm.com> wrote: >> From: Richard Biener [mailto:richard.guent...@gmail.com] >> >> 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). > > Ah yes, right. > >> >> + /* 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. > > Indeed, I understood it for convert_via (the requirement to be > unsigned) but got it wrong here. > >> >> 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). > > Because of the type of the shift constant, the bitwise expression > is usually of type int. However, if you write a function that does > a 32 bit load in host endianness (like a 32 bit little endian load on > x86_64) with a result of size 64 bits, then you need to sign > extend, since the source type is signed. This is a situation I > encountered in bfd_getl32 in binutils I think. > > Now if you consider bfd_getl16 instead a direct sign extension > is out of the question. Suppose bfd_getl16 is called to read from > a memory address that contains 0xff 0xfe. The bitwise expression > would thus be equivalent to the value 0x0000feff since it's of type > int. Then after the sign extension to 64bits you'd have > 0x000000000000feff. But after replacing the bitwise expression > you end up with a 16bit load into a 16bit SSA variable. If you sign > extend that directly to 64 bits you'll end up with > 0xfffffffffffffeff which is wrong. But if you zero extend to an > int value (the type of the bitwise OR expression) and then sign > extend to the target type you'll have the correct result.
Err, but if you zero-extend directly to the target type you have the correct result, too. > But you're right, we can do simpler by sign extending if > load size == size of bitwise expression and zero extend else. The > change of load_type to range_type would still be needed > because in case of a load + bswap it's better to load in the > same type as the type of the parameter of bswap. After bswap > you'd need to convert to a signed or unsigned value according > to the logic above (load size == size of bitwise expression) > > In the original statement, the bitwise OR > expression would have the 2 bytes of higher weight be 0 while > the 2 bytes of lower weight would be the value read from > memory. The result of the sign extension would be > >> >> So I think that the testcase in the patch is fixed already by >> doing the n->type change (and a proper sign-extension detection). > > I don't remember exactly but I think it didn't fix this bug > (but it is a necessary fix anyway). > >> >> Can you please split that part out? > > Sure, that part would need to be applied on gcc 4.9 too. I'll try > to construct a testcase for that. > >> >> That range_type and convert_via looks wrong and unnecessary to me, >> and it doesn't look like you have a testcase excercising it? > > See above. But nothing for the testsuite? The testcase you add fails foul of sign-extending the loads. Richard. >
