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

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

[Roger Sayle]

[-- Attachment #1: Type: text/plain, Size: 3398 bytes --]


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?

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


[-- Attachment #2: patchc2.txt --]
[-- Type: text/plain, Size: 914 bytes --]

diff --git a/gcc/match.pd b/gcc/match.pd
index 008f775..c6cc967 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3401,13 +3401,15 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
      (cmp @0 @2)))))
 
 /* Both signed and unsigned lshift produce the same result, so use
-   the form that minimizes the number of conversions.  */
+   the form that minimizes the number of conversions.  Postpone this
+   transformation until after shifts by zero have been folded.  */
 (simplify
  (convert (lshift:s@0 (convert:s@1 @2) INTEGER_CST@3))
  (if (INTEGRAL_TYPE_P (type)
       && tree_nop_conversion_p (type, TREE_TYPE (@0))
       && INTEGRAL_TYPE_P (TREE_TYPE (@2))
-      && TYPE_PRECISION (TREE_TYPE (@2)) <= TYPE_PRECISION (type))
+      && TYPE_PRECISION (TREE_TYPE (@2)) <= TYPE_PRECISION (type)
+      && !integer_zerop (@3))
   (lshift (convert @2) @3)))
 
 /* Simplifications of conversions.  */

[-- Attachment #3: Wint-in-bool-context-4.c --]
[-- Type: text/plain, Size: 690 bytes --]

/* PR c/102245 */
/* { dg-options "-Wint-in-bool-context" } */
/* { dg-do compile } */

_Bool test1(_Bool x)
{
  return !(x << 0);  /* { dg-warning "boolean context" } */
}

_Bool test2(_Bool x)
{
  return !(x << 1);  /* { dg-warning "boolean context" } */
}

_Bool test3(_Bool x, int y)
{
  return !(x << y);  /* { dg-warning "boolean context" } */
}

_Bool test4(int x, int y)
{
  return !(x << y);  /* { dg-warning "boolean context" } */
}

_Bool test5(int x, int y)
{
  return !((x << y) << 0);  /* { dg-warning "boolean context" } */
}

int test6(_Bool x)
{
  int v = 0;
  return (v & ~1L) | (1L & (x << 0));  /* { dg-bogus "boolean context" } */
}

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

[Richard Biener]

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
>