Re: [PATCH #2] PR c/102245: Disable sign-changing optimization for shifts by zero.

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Sep 14, 2021 at 9:44 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> Respecting Jakub's suggestion that it may be better to warn-on-valid for
> "if (x << 0)" as the author might have intended "if (x < 0)" [which will
> also warn when x is _Bool], the simplest way to resolve this regression
> is to disable the recently added fold transformation for shifts by zero;
> these will be optimized later (elsewhere).  Guarding against integer_zerop
> is the simplest of three alternatives; the second being to only apply
> this transformation to GIMPLE and not GENERIC, and the third (potentially)
> being to explicitly handle shifts by zero here, with an (if cond then else),
> optimizing the expression to a convert, but awkwardly duplicating the
> more general transformation earlier in match.pd's shift simplifications.
>
> This patch has been tested (against a recent snapshot without build issues)
> on x86_64-pc-linux-gnu with "make bootstrap" and "make -k check" with no
> new failures.  Note that test1 in the new testcase is changed from
> dg-bogus to dg-warning compared with version #1.  Ok for mainline?

OK.

Thanks,
Richard.

> 2021-09-14  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         PR c/102245
>         * match.pd (shift optimizations): Disable recent sign-changing
>         optimization for shifts by zero, these will be folded later.
>
> gcc/testsuite/ChangeLog
>         PR c/102245
>         * gcc.dg/Wint-in-bool-context-4.c: New test case.
>
>
> Roger
>
> -----Original Message-----
> From: Jakub Jelinek <jakub@redhat.com>
> Sent: 13 September 2021 11:58
> To: Roger Sayle <roger@nextmovesoftware.com>
> Cc: 'GCC Patches' <gcc-patches@gcc.gnu.org>
> Subject: Re: [PATCH] PR c/102245: Don't warn that ((_Bool)x<<0) isn't a truthvalue.
>
> On Mon, Sep 13, 2021 at 11:42:08AM +0100, Roger Sayle wrote:
> > gcc/c-family/ChangeLog
> >       PR c/102245
> >       * c-common.c (c_common_truthvalue_conversion) [LSHIFT_EXPR]:
> >       Special case (optimize) shifts by zero.
> >
> > gcc/testsuite/ChangeLog
> >       PR c/102245
> >       * gcc.dg/Wint-in-bool-context-4.c: New test case.
> >
> > Roger
> > --
> >
>
> > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index
> > 017e415..44b5fcc 100644
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -3541,6 +3541,10 @@ c_common_truthvalue_conversion (location_t location, tree expr)
> >        break;
> >
> >      case LSHIFT_EXPR:
> > +      /* Treat shifts by zero as a special case.  */
> > +      if (integer_zerop (TREE_OPERAND (expr, 1)))
> > +     return c_common_truthvalue_conversion (location,
> > +                                            TREE_OPERAND (expr, 0));
> >        /* We will only warn on signed shifts here, because the majority of
> >        false positive warnings happen in code where unsigned arithmetic
> >        was used in anticipation of a possible overflow.
>
> > /* PR c/102245 */
> > /* { dg-options "-Wint-in-bool-context" } */
> > /* { dg-do compile } */
> >
> > _Bool test1(_Bool x)
> > {
> >   return !(x << 0);  /* { dg-bogus "boolean context" } */ }
>
> While this exact case is unlikely a misspelling of !(x < 0) as no _Bool is less than zero and hopefully we get a warning for !(x < 0), what about _Bool test1a(int x) {
>   return !(x << 0);
> }
> ?  I think there is a non-zero chance this was meant to be !(x < 0) and the current
> pr102245.c: In function ‘test1a’:
> pr102245.c:3:14: warning: ‘<<’ in boolean context, did you mean ‘<’? [-Wint-in-bool-context]
>     3 |   return !(x << 0);
>       |           ~~~^~~~~
> warning seems to be useful.
>
>         Jakub
>