> 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. 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.
