Re: [Patch, vrp] Allow VRP type conversion folding only for widenings upto word mode

[Richard Biener <rguenther at suse dot de>]

On Sat, 14 Nov 2015, Senthil Kumar Selvaraj wrote:

> On Sat, Nov 14, 2015 at 09:57:40AM +0100, Richard Biener wrote:
> > On November 14, 2015 9:49:28 AM GMT+01:00, Senthil Kumar Selvaraj <senthil_kumar.selvaraj@atmel.com> wrote:
> > >On Sat, Nov 14, 2015 at 09:13:41AM +0100, Marc Glisse wrote:
> > >> On Sat, 14 Nov 2015, Senthil Kumar Selvaraj wrote:
> > >> 
> > >> >This patch came out of a discussion held in the gcc mailing list
> > >> >(https://gcc.gnu.org/ml/gcc/2015-11/msg00067.html).
> > >> >
> > >> >The patch restricts folding of conditional exprs with lhs previously
> > >> >set by a type conversion to occur only if the source of the type
> > >> >conversion's mode is word mode or smaller.
> > >> >
> > >> >Bootstrapped and reg tested on x86_64 (with
> > >--enable-languages=c,c++).
> > >> >
> > >> >If ok, could you commit please? I don't have commit access.
> > >> >
> > >> >Regards
> > >> >Senthil
> > >> >
> > >> >gcc/ChangeLog
> > >> >
> > >> >2015-11-11  Senthil Kumar Selvaraj 
> > ><senthil_kumar.selvaraj@atmel.com>
> > >> >
> > >> >	* tree-vrp.c (simplify_cond_using_ranges): Fold only
> > >> >	if innerop's mode is word_mode or smaller.
> > >> >
> > >> >
> > >> >diff --git gcc/tree-vrp.c gcc/tree-vrp.c
> > >> >index e2393e4..c139bc6 100644
> > >> >--- gcc/tree-vrp.c
> > >> >+++ gcc/tree-vrp.c
> > >> >@@ -9467,6 +9467,8 @@ simplify_cond_using_ranges (gcond *stmt)
> > >> >      innerop = gimple_assign_rhs1 (def_stmt);
> > >> >
> > >> >      if (TREE_CODE (innerop) == SSA_NAME
> > >> >+         && (GET_MODE_SIZE(TYPE_MODE(TREE_TYPE(innerop)))
> > >> >+           <= GET_MODE_SIZE(word_mode))
> > >> >	  && !POINTER_TYPE_P (TREE_TYPE (innerop)))
> > >> >	{
> > >> >	  value_range *vr = get_value_range (innerop);
> > >> 
> > >> I thought the result of the discussion was that the transformation is
> > >ok if
> > >> either it is narrowing or it widens but to something no bigger than
> > >> word_mode. So you should have 2 comparisons, or 1 with a max.
> > >
> > >Hmm, I came to the opposite conclusion - I thought Richard only okayed
> > >"widening upto word-mode", not the narrowing. 
> > 
> > I didn't mean to suggest narrowing is not OK.  In fact narrowing is always OK.
> 
> My bad. Here's a revised patch that checks for both conditions, using
> max as Marc suggested to limit to word_mode or narrowing conversions.
> 
> Bootstrapped and regtested for x86_64 with c and c++.
> 
> Is this ok? If yes, would you commit it
> for me please? I don't have commit access.
> 
> gcc/ChangeLog
> 2015-11-14  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
> 
> 	* tree-vrp.c (simplify_cond_using_ranges): Fold only
> 	if innerop's mode smaller or equal to word_mode or op0's mode.
> 
> 
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index e2393e4..cfd90e7 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -9467,7 +9467,10 @@ simplify_cond_using_ranges (gcond *stmt)
>        innerop = gimple_assign_rhs1 (def_stmt);
>  
>        if (TREE_CODE (innerop) == SSA_NAME
> -	  && !POINTER_TYPE_P (TREE_TYPE (innerop)))
> +	  && !POINTER_TYPE_P (TREE_TYPE (innerop))
> +         && (GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (innerop)))
> +           <= std::max (GET_MODE_SIZE (word_mode),
> +                        GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (op0))))))

Please use TYPE_PRECISION (...) and GET_MODE_PRECISION (word_mode) and
add a comment as to what we are testing here and why.

Btw, ideally we'd factor out a

bool
desired_pro_or_demotion_p (tree to_type, tree from_type) {}

function somewhere as we have similar tests throughout the compiler
that we might want to unify (and also have a central place to
eventually add a target hook if ever desired).

In fact in other places we also check that the type we promote/demote
to matches its mode precision or the type we promote/demote from
already does not.

I'd suggest tree.[ch] for that function.

Please also add a testcase.

Thanks,
Richard.

>  	{
>  	  value_range *vr = get_value_range (innerop);
>  
> 
> Regards
> Senthil
> > 
> > Richard.
> > 
> > >Richard?
> > >
> > >Regards
> > >Senthil
> > >> 
> > >> -- 
> > >> Marc Glisse
> > 
> > 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)