This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Restrict jump threading statement simplifier to scalar types (PR71077)
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