On Thu, 8 Dec 2016, Jakub Jelinek wrote:
> Hi!
>
> The following testcases show two bugs in zero_one_operation (used during
> undistribute optimization) and functions it calls.
> The first one (wrong-code) is fixed by the
> + tree orig_def = *def;
> and
> - if (stmts_to_fix.length () > 0)
> + if (stmts_to_fix.length () > 0 || *def == orig_def)
> changes in zero_one_operation - e.g. in the testcase we have before reassoc
> _6 = _5 * a_11;
> _7 = _5 * _6;
> and call zero_one_operation for _7 with op a_11. The last stmt isn't pushed
> into stmts_to_fix, the *def stmt is handled specially in
> make_new_ssa_for_all_defs. And the previous one, while it is pushed, it is
> popped from there again, because the stmt is removed and instead _6 use
> is replaced with _5. That changes what value _7 holds, so we want to use
> a new SSA_NAME for it (or deal with debug stmts and reset flow sensitive
> info etc.).
>
> The second one is wrong-debug, make_new_ssa_for_def replaces the uses of old
> lhs with the new lhs (which is fine in the code uses, but for debug info
> because we zeroed out a_11 we want to say that previous uses of _7 are now
> the _7's replacement * a_11. Which is what the rest of the patch does
> and there is a guality testcase that verifies it (previously it would fail).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
> 2016-12-08 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/78726
> * tree-ssa-reassoc.c (make_new_ssa_for_def): Add OPCODE and OP
> argument. For lhs uses in debug stmts, don't replace lhs with
> new_lhs, but with a debug temp set to new_lhs opcode op.
> (make_new_ssa_for_all_defs): Add OPCODE argument, pass OPCODE and
> OP down to make_new_ssa_for_def.
> (zero_one_operation): Call make_new_ssa_for_all_defs even when
> stmts_to_fix is empty, if *def has not changed yet. Pass
> OPCODE to make_new_ssa_for_all_defs.
>
> * gcc.c-torture/execute/pr78726.c: New test.
> * gcc.dg/guality/pr78726.c: New test.
>
> --- gcc/tree-ssa-reassoc.c.jj 2016-11-09 16:34:58.000000000 +0100
> +++ gcc/tree-ssa-reassoc.c 2016-12-08 15:53:13.768894146 +0100
> @@ -1153,12 +1153,12 @@ decrement_power (gimple *stmt)
> SSA. Also return the new SSA. */
>
> static tree
> -make_new_ssa_for_def (gimple *stmt)
> +make_new_ssa_for_def (gimple *stmt, enum tree_code opcode, tree op)
> {
> gimple *use_stmt;
> use_operand_p use;
> imm_use_iterator iter;
> - tree new_lhs;
> + tree new_lhs, new_debug_lhs = NULL_TREE;
> tree lhs = gimple_get_lhs (stmt);
>
> new_lhs = make_ssa_name (TREE_TYPE (lhs));
> @@ -1167,8 +1167,28 @@ make_new_ssa_for_def (gimple *stmt)
> /* Also need to update GIMPLE_DEBUGs. */
> FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
> {
> + tree repl = new_lhs;
> + if (is_gimple_debug (use_stmt))
> + {
> + if (new_debug_lhs == NULL_TREE)
> + {
> + new_debug_lhs = make_node (DEBUG_EXPR_DECL);
> + gdebug *def_temp
> + = gimple_build_debug_bind (new_debug_lhs,
> + build2 (opcode, TREE_TYPE (lhs),
> + new_lhs, op),
> + stmt);
> + DECL_ARTIFICIAL (new_debug_lhs) = 1;
> + TREE_TYPE (new_debug_lhs) = TREE_TYPE (lhs);
> + SET_DECL_MODE (new_debug_lhs, TYPE_MODE (TREE_TYPE (lhs)));
> + gimple_set_uid (def_temp, gimple_uid (stmt));
> + gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> + gsi_insert_after (&gsi, def_temp, GSI_SAME_STMT);
> + }
> + repl = new_debug_lhs;
> + }
> FOR_EACH_IMM_USE_ON_STMT (use, iter)
> - SET_USE (use, new_lhs);
> + SET_USE (use, repl);
> update_stmt (use_stmt);
> }
> return new_lhs;
> @@ -1179,7 +1199,7 @@ make_new_ssa_for_def (gimple *stmt)
> if *DEF is not OP. */
>
> static void
> -make_new_ssa_for_all_defs (tree *def, tree op,
> +make_new_ssa_for_all_defs (tree *def, enum tree_code opcode, tree op,
> vec<gimple *> &stmts_to_fix)
> {
> unsigned i;
> @@ -1189,10 +1209,10 @@ make_new_ssa_for_all_defs (tree *def, tr
> && TREE_CODE (*def) == SSA_NAME
> && (stmt = SSA_NAME_DEF_STMT (*def))
> && gimple_code (stmt) != GIMPLE_NOP)
> - *def = make_new_ssa_for_def (stmt);
> + *def = make_new_ssa_for_def (stmt, opcode, op);
>
> FOR_EACH_VEC_ELT (stmts_to_fix, i, stmt)
> - make_new_ssa_for_def (stmt);
> + make_new_ssa_for_def (stmt, opcode, op);
> }
>
> /* Find the single immediate use of STMT's LHS, and replace it
> @@ -1232,6 +1252,7 @@ propagate_op_to_single_use (tree op, gim
> static void
> zero_one_operation (tree *def, enum tree_code opcode, tree op)
> {
> + tree orig_def = *def;
> gimple *stmt = SSA_NAME_DEF_STMT (*def);
> /* PR72835 - Record the stmt chain that has to be updated such that
> we dont use the same LHS when the values computed are different. */
> @@ -1335,8 +1356,8 @@ zero_one_operation (tree *def, enum tree
> }
> while (1);
>
> - if (stmts_to_fix.length () > 0)
> - make_new_ssa_for_all_defs (def, op, stmts_to_fix);
> + if (stmts_to_fix.length () > 0 || *def == orig_def)
> + make_new_ssa_for_all_defs (def, opcode, op, stmts_to_fix);
> }
>
> /* Returns true if statement S1 dominates statement S2. Like
> --- gcc/testsuite/gcc.c-torture/execute/pr78726.c.jj 2016-12-08
> 15:55:08.847434702 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr78726.c 2016-12-08
> 15:54:46.000000000 +0100
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/78726 */
> +
> +unsigned char b = 36, c = 173;
> +unsigned int d;
> +
> +__attribute__((noinline, noclone)) void
> +foo (void)
> +{
> + unsigned a = ~b;
> + d = a * c * c + 1023094746U * a;
> +}
> +
> +int
> +main ()
> +{
> + if (__SIZEOF_INT__ != 4 || __CHAR_BIT__ != 8)
> + return 0;
> + asm volatile ("" : : "g" (&b), "g" (&c) : "memory");
> + foo ();
> + if (d != 799092689U)
> + __builtin_abort ();
> + return 0;
> +}
> --- gcc/testsuite/gcc.dg/guality/pr78726.c.jj 2016-12-08 15:57:07.779926381
> +0100
> +++ gcc/testsuite/gcc.dg/guality/pr78726.c 2016-12-08 16:02:57.216494771
> +0100
> @@ -0,0 +1,30 @@
> +/* PR tree-optimization/78726 */
> +/* { dg-do run } */
> +/* { dg-options "-g" } */
> +
> +#include "../nop.h"
> +
> +unsigned char b = 36, c = 173;
> +unsigned int d;
> +
> +__attribute__((noinline, noclone)) void
> +foo (void)
> +{
> + unsigned a = ~b;
> + unsigned d1 = a * c; /* { dg-final { gdb-test 21 "d1" "~36U
> * 173" } } */
> + unsigned d2 = d1 * c; /* { dg-final { gdb-test 21 "d2" "~36U
> * 173 * 173" } } */
> + unsigned d3 = 1023094746 * a; /* { dg-final { gdb-test 21 "d3" "~36U
> * 1023094746" } } */
> + d = d2 + d3;
> + unsigned d4 = d1 * 2; /* { dg-final { gdb-test 21 "d4" "~36U * 173 *
> 2" } } */
> + unsigned d5 = d2 * 2; /* { dg-final { gdb-test 21 "d5" "~36U
> * 173 * 173 * 2" } } */
> + unsigned d6 = d3 * 2; /* { dg-final { gdb-test 21 "d6" "~36U
> * 1023094746 * 2" } } */
> + asm (NOP : : : "memory");
> +}
> +
> +int
> +main ()
> +{
> + asm volatile ("" : : "g" (&b), "g" (&c) : "memory");
> + foo ();
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
