On November 26, 2014 7:49:55 PM CET, Jakub Jelinek <ja...@redhat.com> wrote: >On Wed, Nov 26, 2014 at 07:20:04PM +0100, Marek Polacek wrote: >> On Wed, Nov 26, 2014 at 06:50:29PM +0100, Jakub Jelinek wrote: >> > On Wed, Nov 26, 2014 at 06:39:44PM +0100, Marek Polacek wrote: >> > > Both C11 and C++14 standards specify that integral promotions are >> > > performed on both operands of a shift-expression. This we do >just >> > > fine. But then we convert the right operand to >integer_type_node. >> > > Not only is this unnecessary, it can also be harfmul, because for >> > > e.g. >> > > void >> > > foo (unsigned int x) >> > > { >> > > if (-1 >> x != -1) > >I'd say fold-const not simplifying this is a bug even if x is signed >int. >I think negative shift counts are undefined even in the middle-end, >Richard?
Well, in the past it has, at least for constant folding, pollibly only in into_const_binop. >Treating 5 >> -5 as 5 << 5 looks just wrong to me, does any FE rely on >that? Not sure. >I don't think the expander will expand it that way though. Not sure :) >Anyway, if the shift count is unnecessary wide type, then the >middle-end >won't be able to optimize it as much as it could if it has been a >narrower >type. > >So, for ubsan purposes (but, shifts are instrumented in the FEs) it is >of >course desirable to see the original type, but perhaps it might be a >good >idea to cast the shift count to integer_type_node say during >gimplification >(perhaps in c_gimplify_expr?). These are all separate issues but I'd rather get rid of conversions than adding some. So I think the original patch is OK. Richard. > Jakub