[vrp] Allow VRP type conversion folding only for widenings upto word mode

Submitted by Senthil Kumar Selvaraj on Nov. 14, 2015, 6:10 p.m.

Details

Message ID 20151114181044.GA1297@jaguar.atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj Nov. 14, 2015, 6:10 p.m.
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.



Regards
Senthil
> 
> Richard.
> 
> >Richard?
> >
> >Regards
> >Senthil
> >> 
> >> -- 
> >> Marc Glisse
> 
>

Comments

Richard Guenther Nov. 16, 2015, 9:02 a.m.
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
> > 
> > 
> 
>

Patch hide | download patch | download mbox

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))))))
 	{
 	  value_range *vr = get_value_range (innerop);