| Message ID | 55D34F59.7020307@sfr.fr |
|---|---|
| State | New |
| Headers | show |
On 08/18/2015 09:29 AM, Mikael Morin wrote: > Le 14/08/2015 09:29, Richard Biener a écrit : >> On Thu, Aug 13, 2015 at 7:47 PM, Mike Stump <mikestump@comcast.net> >> wrote: >>> On Aug 13, 2015, at 3:05 AM, Richard Biener >>> <richard.guenther@gmail.com> wrote: >>>> Ok, then guard the << >> with __GCC__ and do the expensive bit stuff >>>> otherwise. Just to cater for other host compilers doing sth unsensibly >>>> implementation defined. >>> > I haven't found __GCC__ used anywhere in the source, but I have found > __GNUC__ both in the source and the documentation, so I have used that. > >>> Ick. The guard should be specific to the implementation defined >>> semantic or undefined semantic, and then when compiling with gcc, we >>> turn it on. My take is that when we do this, we should add the 5 or >>> 10 other most popular compilers to the list of how they behave so >>> that they all do the cheap path code as well. If the language >>> standard were serious in this area, it would specify a header file >>> that can define the implementation defined and undefined semantics >>> that are interesting for portable code. It isn’t. If it were, we >>> would then just use the standard defined guards. >> > I have used the __GNUC__ macro instead of a direct feature test or a > list of the 10 most common compilers, to avoid the #else branch being > dead basically everywhere. Maybe the #else branch should just be dropped. > >> GCC is always used to build stage2+ so I don't see the need to handle >> alternate host compilers. >> > > Bootstrapped and regression tested on x86_64-pc-linux-gnu. > Also bootstrapped with clang, but I don't think it means anything. > What do you think, OK? > > Mikael > > > > noub_sext_2.CL > > > 2015-08-18 Mikael Morin<mikael@gcc.gnu.org> > > PR other/67042 > * hwint.h (sext_hwi): Switch to unsigned for the left shift, and > conditionalize the whole on __GNUC__. Add fallback code > depending neither on undefined nor implementation-defined behaviour. > I think there was some folks that wanted to see a test for whether or not the faster version could be used during a stage1 build with a non-gcc compiler. That would presumably be some kind of autoconf test to see what happens when shifting into the sign bit. But I think that'd be a hell of a test to write in practice. I'm sure the simple test would work, but it's far more important to know what the compiler is willing to guarantee in the general sense, particularly the interactions with the optimizers and vrp-like analysis. I'm OK with the __GNUC__ conditional. OK for the trunk. Thanks, Jeff
diff --git a/gcc/hwint.h b/gcc/hwint.h index 3793986..4acbf8e 100644 --- a/gcc/hwint.h +++ b/gcc/hwint.h @@ -244,11 +244,27 @@ sext_hwi (HOST_WIDE_INT src, unsigned int prec) if (prec == HOST_BITS_PER_WIDE_INT) return src; else +#if defined (__GNUC__) { + /* Take the faster path if the implementation-defined bits it's relying + on are implemented the way we expect them to be. Namely, conversion + from unsigned to signed preserves bit pattern, and right shift of + a signed value propagates the sign bit. + We have to convert from signed to unsigned and back, because when left + shifting signed values, any overflow is undefined behaviour. */ gcc_checking_assert (prec < HOST_BITS_PER_WIDE_INT); int shift = HOST_BITS_PER_WIDE_INT - prec; - return (src << shift) >> shift; + return ((HOST_WIDE_INT) ((unsigned HOST_WIDE_INT) src << shift)) >> shift; } +#else + { + /* Fall back to the slower, well defined path otherwise. */ + gcc_checking_assert (prec < HOST_BITS_PER_WIDE_INT); + HOST_WIDE_INT sign_mask = HOST_WIDE_INT_1 << (prec - 1); + HOST_WIDE_INT value_mask = (HOST_WIDE_INT_1U << prec) - HOST_WIDE_INT_1U; + return (((src & value_mask) ^ sign_mask) - sign_mask); + } +#endif } /* Zero extend SRC starting from PREC. */