Restrict jump threading statement simplifier to scalar types (PR71077)

On Fri, 19 Aug 2016, Yuri Rumyantsev wrote:

> Hi,
> 
> Here is a simple test-case to reproduce 176.gcc failure (I run it on
> Haswell machine).
> Using 20160819 compiler build we get:
> gcc -O3 -m32 -mavx2 test.c -o test.ref.exe
> /users/ysrumyan/isse_6866$ ./test.ref.exe
> Aborted (core dumped)
> 
> If I apply patch proposed by Patrick test runs properly
> Instead of running we can check number of .jump thread.

Thanks!  With this test case I was able to identify the cause of the
wrong code generation.

Turns out that the problem lies in fold-const.c.  It does not correctly
fold VECTOR_CST comparisons that have a scalar boolean result type.  In
particular fold_binary folds the comparison {1,1,1} != {0,0,0} to false
which causes the threader to register an incorrect jump thread.  In
general VEC1 != VEC2 gets folded as if it were VEC1 == VEC2.

This patch fixes the problematic folding logic.

Does this look OK to commit after bootstrap + regtesting?  The faulty
logic was introduced by the fix for PR68542 (r232518) so I think it is
present on the 6 branch as well.

gcc/ChangeLog:

	PR tree-optimization/71044
	PR tree-optimization/68542
	* fold-const.c (fold_relational_const): Fix folding of
	VECTOR_CST comparisons that have a scalar boolean result type.
	(test_vector_folding): New static function.
	(fold_const_c_tests): Call it.
---
 gcc/fold-const.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

On Fri, 2016-08-19 at 19:25 -0400, Patrick Palka wrote:
> On Fri, 19 Aug 2016, Yuri Rumyantsev wrote:
> 
> > Hi,
> > 
> > Here is a simple test-case to reproduce 176.gcc failure (I run it
> > on
> > Haswell machine).
> > Using 20160819 compiler build we get:
> > gcc -O3 -m32 -mavx2 test.c -o test.ref.exe
> > /users/ysrumyan/isse_6866$ ./test.ref.exe
> > Aborted (core dumped)
> > 
> > If I apply patch proposed by Patrick test runs properly
> > Instead of running we can check number of .jump thread.
> 
> Thanks!  With this test case I was able to identify the cause of the
> wrong code generation.
> 
> Turns out that the problem lies in fold-const.c.  It does not
> correctly
> fold VECTOR_CST comparisons that have a scalar boolean result type. 
>  In
> particular fold_binary folds the comparison {1,1,1} != {0,0,0} to
> false
> which causes the threader to register an incorrect jump thread.  In
> general VEC1 != VEC2 gets folded as if it were VEC1 == VEC2.
> 
> This patch fixes the problematic folding logic.
> 
> Does this look OK to commit after bootstrap + regtesting?  The faulty
> logic was introduced by the fix for PR68542 (r232518) so I think it
> is
> present on the 6 branch as well.
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/71044
> 	PR tree-optimization/68542
> 	* fold-const.c (fold_relational_const): Fix folding of
> 	VECTOR_CST comparisons that have a scalar boolean result type.
> 	(test_vector_folding): New static function.
> 	(fold_const_c_tests): Call it.

FWIW I don't know if we have any policy about this, but I've been
spelling out namespaces such as "selftest" in ChangeLog entries, in the
hope that it makes it easier to distinguish the "real" code from the
selftests.  So the above might read:

gcc/ChangeLog:

	PR tree-optimization/71044
	PR tree-optimization/68542
	* fold-const.c (fold_relational_const): Fix folding of
	VECTOR_CST comparisons that have a scalar boolean result type.
	(selftest::test_vector_folding): New static function.
	(selftest::fold_const_c_tests): Call it.

Also, if there's already a source file that reproduces the issue, is it
worth turning it into a DejaGnu test to complement the selftests?  (for
both end-to-end testing *and* unit-testing, a "belt and braces"
approach).

Hope this is constructive
Dave

> ---
>  gcc/fold-const.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index 30c1e0d..0271ac3 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -13897,7 +13897,6 @@ fold_relational_const (enum tree_code code,
> tree type, tree op0, tree op1)
>        if (!VECTOR_TYPE_P (type))
>  	{
>  	  /* Have vector comparison with scalar boolean result.  */
> -	  bool result = true;
>  	  gcc_assert ((code == EQ_EXPR || code == NE_EXPR)
>  		      && VECTOR_CST_NELTS (op0) == VECTOR_CST_NELTS
> (op1));
>  	  for (unsigned i = 0; i < VECTOR_CST_NELTS (op0); i++)
> @@ -13905,11 +13904,12 @@ fold_relational_const (enum tree_code code,
> tree type, tree op0, tree op1)
>  	      tree elem0 = VECTOR_CST_ELT (op0, i);
>  	      tree elem1 = VECTOR_CST_ELT (op1, i);
>  	      tree tmp = fold_relational_const (code, type, elem0,
> elem1);
> -	      result &= integer_onep (tmp);
> +	      if (tmp == NULL_TREE)
> +		return NULL_TREE;
> +	      if (integer_zerop (tmp))
> +		return constant_boolean_node (false, type);
>  	    }
> -	  if (code == NE_EXPR)
> -	    result = !result;
> -	  return constant_boolean_node (result, type);
> +	  return constant_boolean_node (true, type);
>  	}
>        unsigned count = VECTOR_CST_NELTS (op0);
>        tree *elts =  XALLOCAVEC (tree, count);
> @@ -14517,12 +14517,32 @@ test_arithmetic_folding ()
>  				   x);
>  }
>  
> +/* Verify that various binary operations on vectors are folded
> +   correctly.  */
> +
> +static void
> +test_vector_folding ()
> +{
> +  tree inner_type = integer_type_node;
> +  tree type = build_vector_type (inner_type, 4);
> +  tree zero = build_zero_cst (type);
> +  tree one = build_one_cst (type);
> +
> +  /* Verify equality tests that return a scalar boolean result.  */
> +  tree res_type = boolean_type_node;
> +  ASSERT_FALSE (integer_nonzerop (fold_build2 (EQ_EXPR, res_type,
> zero, one)));
> +  ASSERT_TRUE (integer_nonzerop (fold_build2 (EQ_EXPR, res_type,
> zero, zero)));
> +  ASSERT_TRUE (integer_nonzerop (fold_build2 (NE_EXPR, res_type,
> zero, one)));
> +  ASSERT_FALSE (integer_nonzerop (fold_build2 (NE_EXPR, res_type,
> one, one)));
> +}
> +
>  /* Run all of the selftests within this file.  */
>  
>  void
>  fold_const_c_tests ()
>  {
>    test_arithmetic_folding ();
> +  test_vector_folding ();
>  }
>  
>  } // namespace selftest

On Fri, 19 Aug 2016, David Malcolm wrote:

> On Fri, 2016-08-19 at 19:25 -0400, Patrick Palka wrote:
> > On Fri, 19 Aug 2016, Yuri Rumyantsev wrote:
> > 
> > > Hi,
> > > 
> > > Here is a simple test-case to reproduce 176.gcc failure (I run it
> > > on
> > > Haswell machine).
> > > Using 20160819 compiler build we get:
> > > gcc -O3 -m32 -mavx2 test.c -o test.ref.exe
> > > /users/ysrumyan/isse_6866$ ./test.ref.exe
> > > Aborted (core dumped)
> > > 
> > > If I apply patch proposed by Patrick test runs properly
> > > Instead of running we can check number of .jump thread.
> > 
> > Thanks!  With this test case I was able to identify the cause of the
> > wrong code generation.
> > 
> > Turns out that the problem lies in fold-const.c.  It does not
> > correctly
> > fold VECTOR_CST comparisons that have a scalar boolean result type. 
> >  In
> > particular fold_binary folds the comparison {1,1,1} != {0,0,0} to
> > false
> > which causes the threader to register an incorrect jump thread.  In
> > general VEC1 != VEC2 gets folded as if it were VEC1 == VEC2.
> > 
> > This patch fixes the problematic folding logic.
> > 
> > Does this look OK to commit after bootstrap + regtesting?  The faulty
> > logic was introduced by the fix for PR68542 (r232518) so I think it
> > is
> > present on the 6 branch as well.
> > 
> > gcc/ChangeLog:
> > 
> > 	PR tree-optimization/71044
> > 	PR tree-optimization/68542
> > 	* fold-const.c (fold_relational_const): Fix folding of
> > 	VECTOR_CST comparisons that have a scalar boolean result type.
> > 	(test_vector_folding): New static function.
> > 	(fold_const_c_tests): Call it.
> 
> FWIW I don't know if we have any policy about this, but I've been
> spelling out namespaces such as "selftest" in ChangeLog entries, in the
> hope that it makes it easier to distinguish the "real" code from the
> selftests.  So the above might read:
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/71044
> 	PR tree-optimization/68542
> 	* fold-const.c (fold_relational_const): Fix folding of
> 	VECTOR_CST comparisons that have a scalar boolean result type.
> 	(selftest::test_vector_folding): New static function.
> 	(selftest::fold_const_c_tests): Call it.

Will do.

> 
> Also, if there's already a source file that reproduces the issue, is it
> worth turning it into a DejaGnu test to complement the selftests?  (for
> both end-to-end testing *and* unit-testing, a "belt and braces"
> approach).

Turning it into a compile test that counts the number of jumps threaded
seems potentially flaky but I'm not against it.  And I'm not sure how to
reliably turn it into an execution test.  Would the directives

/* { dg-do run }  */
/* { dg-require-effective-target avx2 }  */
/* { dg-require-effective-target ia32 }  */
/* { dg-options "-O3 -mavx2" }  */

work?

> 
> Hope this is constructive
> Dave
> 
> > ---
> >  gcc/fold-const.c | 30 +++++++++++++++++++++++++-----
> >  1 file changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> > index 30c1e0d..0271ac3 100644
> > --- a/gcc/fold-const.c
> > +++ b/gcc/fold-const.c
> > @@ -13897,7 +13897,6 @@ fold_relational_const (enum tree_code code,
> > tree type, tree op0, tree op1)
> >        if (!VECTOR_TYPE_P (type))
> >  	{
> >  	  /* Have vector comparison with scalar boolean result.  */
> > -	  bool result = true;
> >  	  gcc_assert ((code == EQ_EXPR || code == NE_EXPR)
> >  		      && VECTOR_CST_NELTS (op0) == VECTOR_CST_NELTS
> > (op1));
> >  	  for (unsigned i = 0; i < VECTOR_CST_NELTS (op0); i++)
> > @@ -13905,11 +13904,12 @@ fold_relational_const (enum tree_code code,
> > tree type, tree op0, tree op1)
> >  	      tree elem0 = VECTOR_CST_ELT (op0, i);
> >  	      tree elem1 = VECTOR_CST_ELT (op1, i);
> >  	      tree tmp = fold_relational_const (code, type, elem0,
> > elem1);
> > -	      result &= integer_onep (tmp);
> > +	      if (tmp == NULL_TREE)
> > +		return NULL_TREE;
> > +	      if (integer_zerop (tmp))
> > +		return constant_boolean_node (false, type);
> >  	    }
> > -	  if (code == NE_EXPR)
> > -	    result = !result;
> > -	  return constant_boolean_node (result, type);
> > +	  return constant_boolean_node (true, type);
> >  	}
> >        unsigned count = VECTOR_CST_NELTS (op0);
> >        tree *elts =  XALLOCAVEC (tree, count);
> > @@ -14517,12 +14517,32 @@ test_arithmetic_folding ()
> >  				   x);
> >  }
> >  
> > +/* Verify that various binary operations on vectors are folded
> > +   correctly.  */
> > +
> > +static void
> > +test_vector_folding ()
> > +{
> > +  tree inner_type = integer_type_node;
> > +  tree type = build_vector_type (inner_type, 4);
> > +  tree zero = build_zero_cst (type);
> > +  tree one = build_one_cst (type);
> > +
> > +  /* Verify equality tests that return a scalar boolean result.  */
> > +  tree res_type = boolean_type_node;
> > +  ASSERT_FALSE (integer_nonzerop (fold_build2 (EQ_EXPR, res_type,
> > zero, one)));
> > +  ASSERT_TRUE (integer_nonzerop (fold_build2 (EQ_EXPR, res_type,
> > zero, zero)));
> > +  ASSERT_TRUE (integer_nonzerop (fold_build2 (NE_EXPR, res_type,
> > zero, one)));
> > +  ASSERT_FALSE (integer_nonzerop (fold_build2 (NE_EXPR, res_type,
> > one, one)));
> > +}
> > +
> >  /* Run all of the selftests within this file.  */
> >  
> >  void
> >  fold_const_c_tests ()
> >  {
> >    test_arithmetic_folding ();
> > +  test_vector_folding ();
> >  }
> >  
> >  } // namespace selftest
>

On Sat, Aug 20, 2016 at 1:25 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Fri, 19 Aug 2016, Yuri Rumyantsev wrote:
>
>> Hi,
>>
>> Here is a simple test-case to reproduce 176.gcc failure (I run it on
>> Haswell machine).
>> Using 20160819 compiler build we get:
>> gcc -O3 -m32 -mavx2 test.c -o test.ref.exe
>> /users/ysrumyan/isse_6866$ ./test.ref.exe
>> Aborted (core dumped)
>>
>> If I apply patch proposed by Patrick test runs properly
>> Instead of running we can check number of .jump thread.
>
> Thanks!  With this test case I was able to identify the cause of the
> wrong code generation.
>
> Turns out that the problem lies in fold-const.c.  It does not correctly
> fold VECTOR_CST comparisons that have a scalar boolean result type.  In
> particular fold_binary folds the comparison {1,1,1} != {0,0,0} to false
> which causes the threader to register an incorrect jump thread.  In
> general VEC1 != VEC2 gets folded as if it were VEC1 == VEC2.
>
> This patch fixes the problematic folding logic.
>
> Does this look OK to commit after bootstrap + regtesting?  The faulty
> logic was introduced by the fix for PR68542 (r232518) so I think it is
> present on the 6 branch as well.

The fold_relational_const changes are ok, please factor in changes to the
testcase / unit-testing as requested.  Ok also for the branch after it re-opens.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         PR tree-optimization/71044
>         PR tree-optimization/68542
>         * fold-const.c (fold_relational_const): Fix folding of
>         VECTOR_CST comparisons that have a scalar boolean result type.
>         (test_vector_folding): New static function.
>         (fold_const_c_tests): Call it.
> ---
>  gcc/fold-const.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index 30c1e0d..0271ac3 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -13897,7 +13897,6 @@ fold_relational_const (enum tree_code code, tree type, tree op0, tree op1)
>        if (!VECTOR_TYPE_P (type))
>         {
>           /* Have vector comparison with scalar boolean result.  */
> -         bool result = true;
>           gcc_assert ((code == EQ_EXPR || code == NE_EXPR)
>                       && VECTOR_CST_NELTS (op0) == VECTOR_CST_NELTS (op1));
>           for (unsigned i = 0; i < VECTOR_CST_NELTS (op0); i++)
> @@ -13905,11 +13904,12 @@ fold_relational_const (enum tree_code code, tree type, tree op0, tree op1)
>               tree elem0 = VECTOR_CST_ELT (op0, i);
>               tree elem1 = VECTOR_CST_ELT (op1, i);
>               tree tmp = fold_relational_const (code, type, elem0, elem1);
> -             result &= integer_onep (tmp);
> +             if (tmp == NULL_TREE)
> +               return NULL_TREE;
> +             if (integer_zerop (tmp))
> +               return constant_boolean_node (false, type);
>             }
> -         if (code == NE_EXPR)
> -           result = !result;
> -         return constant_boolean_node (result, type);
> +         return constant_boolean_node (true, type);
>         }
>        unsigned count = VECTOR_CST_NELTS (op0);
>        tree *elts =  XALLOCAVEC (tree, count);
> @@ -14517,12 +14517,32 @@ test_arithmetic_folding ()
>                                    x);
>  }
>
> +/* Verify that various binary operations on vectors are folded
> +   correctly.  */
> +
> +static void
> +test_vector_folding ()
> +{
> +  tree inner_type = integer_type_node;
> +  tree type = build_vector_type (inner_type, 4);
> +  tree zero = build_zero_cst (type);
> +  tree one = build_one_cst (type);
> +
> +  /* Verify equality tests that return a scalar boolean result.  */
> +  tree res_type = boolean_type_node;
> +  ASSERT_FALSE (integer_nonzerop (fold_build2 (EQ_EXPR, res_type, zero, one)));
> +  ASSERT_TRUE (integer_nonzerop (fold_build2 (EQ_EXPR, res_type, zero, zero)));
> +  ASSERT_TRUE (integer_nonzerop (fold_build2 (NE_EXPR, res_type, zero, one)));
> +  ASSERT_FALSE (integer_nonzerop (fold_build2 (NE_EXPR, res_type, one, one)));
> +}
> +
>  /* Run all of the selftests within this file.  */
>
>  void
>  fold_const_c_tests ()
>  {
>    test_arithmetic_folding ();
> +  test_vector_folding ();
>  }
>
>  } // namespace selftest
> --
> 2.9.3.650.g20ba99f
>