On 10/08/16 18:57, Jakub Jelinek wrote:
On Wed, Aug 10, 2016 at 08:51:32AM +1000, kugan wrote:
I see it now. The problem is we are just looking at (-1) being in the ops
list for passing changed to rewrite_expr_tree in the case of multiplication
by negate. If we have combined (-1), as in the testcase, we will not have
the (-1) and will pass changed=false to rewrite_expr_tree.
We should set changed based on what happens in try_special_add_to_ops.
Attached patch does this. Bootstrap and regression testing are ongoing. Is
this OK for trunk if there is no regression.
I think the bug is elsewhere. In particular in
undistribute_ops_list/zero_one_operation/decrement_power.
All those look problematic in this regard, they change RHS of statements
to something that holds a different value, while keeping the LHS.
So, generally you should instead just add a new stmt next to the old one,
and adjust data structures (replace the old SSA_NAME in some ->op with
the new one). decrement_power might be a problem here, dunno if all the
builtins are const in all cases that DSE would kill the old one,
Richard, any preferences for that? reset flow sensitive info + reset debug
stmt uses, or something different? Though, replacing the LHS with a new
anonymous SSA_NAME might be needed too, in case it is before SSA_NAME of a
user var that doesn't yet have any debug stmts.
Hi Jakub,
This is the patch I have now (not full tested yet). This is along what
you described above. I think I have to handle all the LHS in
zero_one_operation too, I will wait for the feedback before working on it.
Thanks,
Kugan
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
index e69de29..049eddc 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr72835.c
@@ -0,0 +1,36 @@
+/* PR tree-optimization/72835. */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+struct struct_1 {
+ unsigned int m1 : 6 ;
+ unsigned int m2 : 24 ;
+ unsigned int m3 : 6 ;
+};
+
+unsigned short var_32 = 0x2d10;
+
+struct struct_1 s1;
+
+void init ()
+{
+ s1.m1 = 4;
+ s1.m2 = 0x7ca4b8;
+ s1.m3 = 24;
+}
+
+void foo ()
+{
+ unsigned int c
+ = ((unsigned int) s1.m2) * (-((unsigned int) s1.m3))
+ + (var_32) * (-((unsigned int) (s1.m1)));
+ if (c != 4098873984)
+ __builtin_abort ();
+}
+
+int main ()
+{
+ init ();
+ foo ();
+ return 0;
+}
diff --git a/gcc/tree-ssa-reassoc.c b/gcc/tree-ssa-reassoc.c
index 7fd7550..b8dfa39 100644
--- a/gcc/tree-ssa-reassoc.c
+++ b/gcc/tree-ssa-reassoc.c
@@ -1039,7 +1039,7 @@ eliminate_using_constants (enum tree_code opcode,
static void linearize_expr_tree (vec<operand_entry *> *, gimple *,
- bool, bool);
+ bool, bool, bool *);
/* Structure for tracking and counting operands. */
struct oecount {
@@ -1183,9 +1183,25 @@ propagate_op_to_single_use (tree op, gimple *stmt, tree
*def)
is updated if there is only one operand but no operation left. */
static void
-zero_one_operation (tree *def, enum tree_code opcode, tree op)
+zero_one_operation (tree *def, enum tree_code opcode, tree op, bool
ops_changed)
{
gimple *stmt = SSA_NAME_DEF_STMT (*def);
+ /* In this case, the result in the *def will be different as
+ compared to how it was. Therefore, to avoid having SSA
+ which will have range_info and debug that reflects old
+ operation, create a new SSA and use it (PR72835). */
+ if (ops_changed)
+ {
+ use_operand_p use_p;
+ gimple *use_stmt;
+ gimple *stmt = SSA_NAME_DEF_STMT (*def);
+ tree new_def = make_ssa_name (TREE_TYPE (*def));
+ gcc_checking_assert (single_imm_use (*def, &use_p, &use_stmt));
+ SET_USE (use_p, new_def);
+ gimple_set_lhs (stmt, new_def);
+ *def = new_def;
+ update_stmt (use_stmt);
+ }
do
{
@@ -1250,6 +1266,15 @@ zero_one_operation (tree *def, enum tree_code opcode,
tree op)
else if (is_gimple_assign (stmt2)
&& gimple_assign_rhs_code (stmt2) == NEGATE_EXPR)
{
+ /* In this case the result in the op will be
+ different as compared to how it was. Therefore, to avoid
+ having SSA which will have range_info and debug that
+ reflects old operation, create a new SSA and use
+ it (PR72835). */
+ tree tmp = make_ssa_name (TREE_TYPE (op));
+ gimple_set_lhs (stmt2, tmp);
+ gimple_assign_set_rhs2 (stmt, tmp);
+ update_stmt (stmt2);
if (gimple_assign_rhs1 (stmt2) == op)
{
tree cst = build_minus_one_cst (TREE_TYPE (op));
@@ -1453,7 +1478,8 @@ build_and_add_sum (tree type, tree op1, tree op2, enum
tree_code opcode)
static bool
undistribute_ops_list (enum tree_code opcode,
- vec<operand_entry *> *ops, struct loop *loop)
+ vec<operand_entry *> *ops, struct loop *loop,
+ bool *ops_changed)
{
unsigned int length = ops->length ();
operand_entry *oe1;
@@ -1521,7 +1547,7 @@ undistribute_ops_list (enum tree_code opcode,
oedef = SSA_NAME_DEF_STMT ((*ops)[i]->op);
oecode = gimple_assign_rhs_code (oedef);
linearize_expr_tree (&subops[i], oedef,
- associative_tree_code (oecode), false);
+ associative_tree_code (oecode), false, ops_changed);
FOR_EACH_VEC_ELT (subops[i], j, oe1)
{
@@ -1617,7 +1643,7 @@ undistribute_ops_list (enum tree_code opcode,
fprintf (dump_file, "Building (");
print_generic_expr (dump_file, oe1->op, 0);
}
- zero_one_operation (&oe1->op, c->oecode, c->op);
+ zero_one_operation (&oe1->op, c->oecode, c->op, *ops_changed);
EXECUTE_IF_SET_IN_BITMAP (candidates2, first+1, i, sbi0)
{
gimple *sum;
@@ -1627,7 +1653,7 @@ undistribute_ops_list (enum tree_code opcode,
fprintf (dump_file, " + ");
print_generic_expr (dump_file, oe2->op, 0);
}
- zero_one_operation (&oe2->op, c->oecode, c->op);
+ zero_one_operation (&oe2->op, c->oecode, c->op, *ops_changed);
sum = build_and_add_sum (TREE_TYPE (oe1->op),
oe1->op, oe2->op, opcode);
oe2->op = build_zero_cst (TREE_TYPE (oe2->op));
@@ -4456,12 +4482,16 @@ acceptable_pow_call (gcall *stmt, tree *base,
HOST_WIDE_INT *exponent)
}
/* Try to derive and add operand entry for OP to *OPS. Return false if
- unsuccessful. */
+ unsuccessful. If we changed the operands such that the (intermediate)
+ results can be different (as in the case of NEGATE_EXPR converted to
+ multiplication by -1), set ops_changed to true so that we will not
+ reuse the SSA (PR72835). */
static bool
try_special_add_to_ops (vec<operand_entry *> *ops,
enum tree_code code,
- tree op, gimple* def_stmt)
+ tree op, gimple* def_stmt,
+ bool *ops_changed)
{
tree base = NULL_TREE;
HOST_WIDE_INT exponent = 0;
@@ -4492,6 +4522,8 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
add_to_ops_vec (ops, rhs1);
add_to_ops_vec (ops, cst);
gimple_set_visited (def_stmt, true);
+ if (ops_changed)
+ *ops_changed = true;
return true;
}
@@ -4499,11 +4531,12 @@ try_special_add_to_ops (vec<operand_entry *> *ops,
}
/* Recursively linearize a binary expression that is the RHS of STMT.
- Place the operands of the expression tree in the vector named OPS. */
+ Place the operands of the expression tree in the vector named OPS.
+ Return TRUE if try_special_add_to_ops has set ops_changed to TRUE. */
static void
linearize_expr_tree (vec<operand_entry *> *ops, gimple *stmt,
- bool is_associative, bool set_visited)
+ bool is_associative, bool set_visited, bool *ops_changed)
{
tree binlhs = gimple_assign_rhs1 (stmt);
tree binrhs = gimple_assign_rhs2 (stmt);
@@ -4547,10 +4580,12 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple
*stmt,
if (!binrhsisreassoc)
{
- if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+ if (!try_special_add_to_ops (ops, rhscode, binrhs,
+ binrhsdef, ops_changed))
add_to_ops_vec (ops, binrhs);
- if (!try_special_add_to_ops (ops, rhscode, binlhs, binlhsdef))
+ if (!try_special_add_to_ops (ops, rhscode, binlhs,
+ binlhsdef, ops_changed))
add_to_ops_vec (ops, binlhs);
return;
@@ -4588,9 +4623,9 @@ linearize_expr_tree (vec<operand_entry *> *ops, gimple
*stmt,
|| !is_reassociable_op (SSA_NAME_DEF_STMT (binrhs),
rhscode, loop));
linearize_expr_tree (ops, SSA_NAME_DEF_STMT (binlhs),
- is_associative, set_visited);
+ is_associative, set_visited, ops_changed);
- if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef))
+ if (!try_special_add_to_ops (ops, rhscode, binrhs, binrhsdef, ops_changed))
add_to_ops_vec (ops, binrhs);
}
@@ -5322,12 +5357,20 @@ reassociate_bb (basic_block bb)
if (TREE_CODE (lhs) == SSA_NAME && has_zero_uses (lhs))
continue;
+ bool ops_changed = false;
gimple_set_visited (stmt, true);
- linearize_expr_tree (&ops, stmt, true, true);
+ linearize_expr_tree (&ops, stmt, true, true, NULL);
ops.qsort (sort_by_operand_rank);
optimize_ops_list (rhs_code, &ops);
+ /* While in undistribute_ops_list, NEGATE_EXPR is factored out,
+ operands to the reassociated stmts will be different
+ compared to how it was. In this case, to avoid having SSA
+ which will have range_info and debug that reflects old
+ operation, rewrite_expr_tree has to be called with
+ changed = true (PR72835). */
if (undistribute_ops_list (rhs_code, &ops,
- loop_containing_stmt (stmt)))
+ loop_containing_stmt (stmt),
+ &ops_changed))
{
ops.qsort (sort_by_operand_rank);
optimize_ops_list (rhs_code, &ops);
@@ -5415,7 +5458,8 @@ reassociate_bb (basic_block bb)
new_lhs = rewrite_expr_tree (stmt, 0, ops,
powi_result != NULL
- || negate_result);
+ || negate_result
+ || ops_changed);
}
/* If we combined some repeated factors into a
