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

[Richard Biener <rguenther@suse.de>]

On Wed, 18 Nov 2015, Senthil Kumar Selvaraj wrote:

> On Mon, Nov 16, 2015 at 10:02:15AM +0100, Richard Biener wrote:
> > 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.
> 
> How does the below patch look? Bootstrapped, but not regtested yet.
> 
> The testcase was rather tricky to write - I wasn't sure how to reliably
> get a type bigger than a word for all targets. I resorted to __int128,
> not sure it's a good idea though - I should probably add dg-skip-if for
> targets that don't support that. Do you know of a better way to write
> that?

Ah, I was hoping for an avr specific testcase (as you have that already)
scanning assembler output - after all we want to check the final result,
not just what VRP did.

> diff --git gcc/tree-vrp.c gcc/tree-vrp.c
> index 5d085b4..4513b88 100644
> --- gcc/tree-vrp.c
> +++ gcc/tree-vrp.c
> @@ -9467,7 +9467,8 @@ 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))
> +         && desired_pro_or_demotion_p (innerop, op0))
>  	{
>  	  value_range *vr = get_value_range (innerop);
>  
> diff --git gcc/tree.h gcc/tree.h
> index 0b9c3b9..91099ed 100644
> --- gcc/tree.h
> +++ gcc/tree.h
> @@ -5316,4 +5316,18 @@ get_decl_source_range (tree decl)
>    return get_range_from_loc (line_table, loc);
>  }
>  
> +/* Return true if it makes sense to promote/demote from_type to to_type. */
> +inline bool
> +desired_pro_or_demotion_p (tree to_type, tree from_type)

Please pass in a type here.

Otherwise ok.

Thanks,
Richard.

> +{
> +  unsigned int to_type_precision = TYPE_PRECISION (TREE_TYPE (to_type));
> +
> +  /* OK to promote if to_type is no bigger than word_mode. */
> +  if (to_type_precision <= GET_MODE_PRECISION (word_mode))
> +    return true;
> +
> +  /* Otherwise, allow only if narrowing or same precision conversions. */
> +  return to_type_precision <= TYPE_PRECISION (TREE_TYPE (from_type));
> +}
> +
>  #endif  /* GCC_TREE_H  */
> 
> > 
> > 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)
> 
> 

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