diff mbox series

Further bootstrap unbreak (was Re: [PATCH] PR90838: Support ctz idioms)

Message ID 20200113085539.GG2416@tucnak
State New
Headers show
Series Further bootstrap unbreak (was Re: [PATCH] PR90838: Support ctz idioms) | expand

Commit Message

Jakub Jelinek Jan. 13, 2020, 8:55 a.m. UTC
On Sat, Jan 11, 2020 at 05:30:52PM +0100, Jakub Jelinek wrote:
> On Sat, Jan 11, 2020 at 05:24:19PM +0100, Andreas Schwab wrote:
> > ../../gcc/tree-ssa-forwprop.c: In function 'bool simplify_count_trailing_zeroes(gimple_stmt_iterator*)':
> > ../../gcc/tree-ssa-forwprop.c:1925:23: error: variable 'mode' set but not used [-Werror=unused-but-set-variable]
> >  1925 |       scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
> >       |                       ^~~~
> 
> Oops, then I think we need following, but can't commit it until Monday.

Unfortunately, at least when testing x86_64-linux to s390x-linux cross,
there are two warnings instead of just one:
../../gcc/tree-ssa-forwprop.c: In function ‘bool simplify_count_trailing_zeroes(gimple_stmt_iterator*)’:
../../gcc/tree-ssa-forwprop.c:1925:23: warning: variable ‘mode’ set but not used [-Wunused-but-set-variable]
       scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
                       ^~~~
../../gcc/tree-ssa-forwprop.c:1932:7: warning: ‘ctzval’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       if (zero_val != ctzval && !(zero_val == 0 && ctzval == type_size))
       ^~
at -O0 and just the first one at -O2.
The first warning (or with error with -Werror) can be cured by the
patch I've posted on Saturday, I've successfully bootstrapped/regtested
it in the mean time on x86_64-linux and i686-linux, tested on aarch64-linux
including the testcase and tested on s390x-linux.

Or we could also initialize ctzval as in the following patch to e.g. match
what we do in vr-values.c with *_DEFINED_VALUE_AT_ZERO.  Except for the
x86_64-linux/i686-linux bootstrap/regtest, tested similarly to the above.

So, ok for trunk and which one?

2020-01-13  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/90838
	* tree-ssa-forwprop.c (simplify_count_trailing_zeroes): Use
	SCALAR_INT_TYPE_MODE directly in CTZ_DEFINED_VALUE_AT_ZERO macro
	argument rather than to initialize temporary for targets that
	don't use the mode argument at all.  Initialize ctzval to avoid
	warning at -O0.


	Jakub

Comments

Richard Biener Jan. 13, 2020, 9:40 a.m. UTC | #1
On Mon, 13 Jan 2020, Jakub Jelinek wrote:

> On Sat, Jan 11, 2020 at 05:30:52PM +0100, Jakub Jelinek wrote:
> > On Sat, Jan 11, 2020 at 05:24:19PM +0100, Andreas Schwab wrote:
> > > ../../gcc/tree-ssa-forwprop.c: In function 'bool simplify_count_trailing_zeroes(gimple_stmt_iterator*)':
> > > ../../gcc/tree-ssa-forwprop.c:1925:23: error: variable 'mode' set but not used [-Werror=unused-but-set-variable]
> > >  1925 |       scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
> > >       |                       ^~~~
> > 
> > Oops, then I think we need following, but can't commit it until Monday.
> 
> Unfortunately, at least when testing x86_64-linux to s390x-linux cross,
> there are two warnings instead of just one:
> ../../gcc/tree-ssa-forwprop.c: In function ‘bool simplify_count_trailing_zeroes(gimple_stmt_iterator*)’:
> ../../gcc/tree-ssa-forwprop.c:1925:23: warning: variable ‘mode’ set but not used [-Wunused-but-set-variable]
>        scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
>                        ^~~~
> ../../gcc/tree-ssa-forwprop.c:1932:7: warning: ‘ctzval’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        if (zero_val != ctzval && !(zero_val == 0 && ctzval == type_size))
>        ^~
> at -O0 and just the first one at -O2.
> The first warning (or with error with -Werror) can be cured by the
> patch I've posted on Saturday, I've successfully bootstrapped/regtested
> it in the mean time on x86_64-linux and i686-linux, tested on aarch64-linux
> including the testcase and tested on s390x-linux.
> 
> Or we could also initialize ctzval as in the following patch to e.g. match
> what we do in vr-values.c with *_DEFINED_VALUE_AT_ZERO.  Except for the
> x86_64-linux/i686-linux bootstrap/regtest, tested similarly to the above.
> 
> So, ok for trunk and which one?

OK, the one below looks good to me.

> 2020-01-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/90838
> 	* tree-ssa-forwprop.c (simplify_count_trailing_zeroes): Use
> 	SCALAR_INT_TYPE_MODE directly in CTZ_DEFINED_VALUE_AT_ZERO macro
> 	argument rather than to initialize temporary for targets that
> 	don't use the mode argument at all.  Initialize ctzval to avoid
> 	warning at -O0.
> 
> --- gcc/tree-ssa-forwprop.c
> +++ gcc/tree-ssa-forwprop.c
> @@ -1920,10 +1920,10 @@ simplify_count_trailing_zeroes (gimple_stmt_iterator *gsi)
>  				      res_ops[1], res_ops[2], zero_val))
>      {
>        tree type = TREE_TYPE (res_ops[0]);
> -      HOST_WIDE_INT ctzval;
> +      HOST_WIDE_INT ctzval = 0;
>        HOST_WIDE_INT type_size = tree_to_shwi (TYPE_SIZE (type));
> -      scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
> -      bool zero_ok = CTZ_DEFINED_VALUE_AT_ZERO (mode, ctzval) == 2;
> +      bool zero_ok
> +	= CTZ_DEFINED_VALUE_AT_ZERO (SCALAR_INT_TYPE_MODE (type), ctzval) == 2;
>  
>        /* Skip if there is no value defined at zero, or if we can't easily
>  	 return the correct value for zero.  */
> 
> 	Jakub
Wilco Dijkstra Jan. 13, 2020, 10:33 a.m. UTC | #2
Hi Jakub,

On Sat, Jan 11, 2020 at 05:30:52PM +0100, Jakub Jelinek wrote:
> On Sat, Jan 11, 2020 at 05:24:19PM +0100, Andreas Schwab wrote:
> > ../../gcc/tree-ssa-forwprop.c: In function 'bool simplify_count_trailing_zeroes(gimple_stmt_iterator*)':
> > ../../gcc/tree-ssa-forwprop.c:1925:23: error: variable 'mode' set but not used [-Werror=unused-but-set-variable]
> >  1925 |       scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
> >       |                       ^~~~
> 
> Oops, then I think we need following, but can't commit it until Monday.

Thanks for sorting this out so quickly, Jakub! It looks like we need to convert these macros
to targetm hooks given it's too difficult to get them to compile without warnings/errors...
That would also allow us to fix the odd interface of CTZ_DEFINED_VALUE_AT_ZERO
and remove the 1 vs 2 distinction.

Cheers,
Wilco
diff mbox series

Patch

--- gcc/tree-ssa-forwprop.c
+++ gcc/tree-ssa-forwprop.c
@@ -1920,10 +1920,10 @@  simplify_count_trailing_zeroes (gimple_stmt_iterator *gsi)
 				      res_ops[1], res_ops[2], zero_val))
     {
       tree type = TREE_TYPE (res_ops[0]);
-      HOST_WIDE_INT ctzval;
+      HOST_WIDE_INT ctzval = 0;
       HOST_WIDE_INT type_size = tree_to_shwi (TYPE_SIZE (type));
-      scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
-      bool zero_ok = CTZ_DEFINED_VALUE_AT_ZERO (mode, ctzval) == 2;
+      bool zero_ok
+	= CTZ_DEFINED_VALUE_AT_ZERO (SCALAR_INT_TYPE_MODE (type), ctzval) == 2;
 
       /* Skip if there is no value defined at zero, or if we can't easily
 	 return the correct value for zero.  */