On Mon, Mar 10, 2025 at 03:51:29PM +0100, Konstantinos Eleftheriou wrote:
> Testcases for patterns `((a ^ b) & c) cmp d | a != b -> (0 cmp d | a != b)`
> and `(a ^ b) cmp c | a != b -> (0 cmp c | a != b)` were failing on some
> targets, like PowerPC.
>
> This patch moves the optimization to reassoc. Doing so, we can now handle
> cases where the related conditions appear in an AND expression too. Also,
> we can optimize cases where we have intermediate expressions between the
> related ones in the AND/OR expression on some targets. This is not handled
> on targets like PowerPC, where each condition of the AND/OR expression
> is placed into a different basic block.
I think this is too late for GCC 15.
Not full review, just some comments.
> Bootstrapped/regtested on x86 and AArch64.
>
> PR tree-optimization/116860
>
> gcc/ChangeLog:
>
> * match.pd: Remove the following patterns:
> ((a ^ b) & c) cmp d | a != b -> (0 cmp d | a != b)
> (a ^ b) cmp c | a != b -> (0 cmp c | a != b)
I'm not convinced you need to remove this from match.pd, I'd keep it
and add the optimization to tree-ssa-reassoc.cc as well.
In match.pd, it can trigger more often than twice for reassoc.
> @@ -4077,6 +4079,347 @@ optimize_range_tests_var_bound (enum tree_code
> opcode, int first, int length,
> return any_changes;
> }
>
> +/* Helper function for optimize_cmp_xor_exprs. Visit EXPR operands
> + recursively and try to find comparison or XOR expressions that can be
> + solved using the expressions in CALC_STMTS. Expressions that can be
> folded
> + to 0 are stored in STMTS_TO_FOLD. IS_OR_EXPR is true for OR expressions
> + and false for AND expressions. */
> +
> +tree
> +solve_expr (tree expr, std::set<gimple *> *calc_stmts,
> + std::set<gimple *> *stmts_to_fold, std::set<tree> *visited,
> + bool is_or_expr)
Why are you using std::set? Normally especially for consistency when gcc
has its own containers we use the gcc containers, not stl ones.
So, I'd use hash_set.
> +{
> + /* Return, if have already visited this expression or the expression is not
> + an SSA name. */
> + if ((visited->find (expr) != visited->end ())
> + || (TREE_CODE (expr) != SSA_NAME))
Why the redundant ()s around the || operands?
> + if (terminal_node_num != (op_num - 1))
Again, why the ()s around op_num - 1.
> + tree_code inverted_code = invert_tree_comparison (stmt_rhs_code,
> + HONOR_NANS (TREE_TYPE (expr)));
This is ugly formatting, just use
tree_code inverted_code
= invert_tree_comparison (stmt_rhs_code,
HONOR_NANS (TREE_TYPE (expr)));
or so.
> + if (is_or_expr
> + || (gimple_bb (stmt) == gimple_bb (def_stmt))
Why the extra ()s?
> + || reassoc_stmt_dominates_stmt_p (stmt, def_stmt))
> + tree op = gimple_op (def_stmt, i);
> + if (!op) continue;
continue should be on a separate line.
> + if (!is_or_expr && (opcode != BIT_AND_EXPR))
> + return false;
Again, don't use () unless it helps readability, avoid warnings
or is needed for formatting.
> + tree op = *(expr_terms.begin ());
Again.
> + if (!last_stmt || (gimple_code (last_stmt) != GIMPLE_COND))
Again.
> + && (EDGE_COUNT (pred->succs) > 1))
Again.
> + std::set<tree> op_terminal_nodes = get_terminal_nodes (op,
> + &terminal_nodes, i);
Too ugly formatting, move = on the next line, then this one will most likely
fit.
> + unsigned int j = i + 1;
> + for (std::set<tree>::iterator next_it = std::next (it);
> + next_it != expr_terms.end (); ++next_it, ++j)
> + {
> + tree next_op = *next_it;
> + std::set<tree> next_op_term_nodes = get_terminal_nodes (next_op,
> + &terminal_nodes, j);
> + std::set<tree> intersection;
> + std::set_intersection (op_terminal_nodes.begin (),
> + op_terminal_nodes.end (),
> + next_op_term_nodes.begin (),
> + next_op_term_nodes.end (),
> + std::inserter (intersection,
> + intersection.begin ()));
> + if (intersection.size () > 1)
I think computing whole intersection only to check if there are 2+ entries
in the intersection is expensive. IMHO just traverse one hash_set and
look up in each case the tree in the other hash_set and stop looking after
finding 2.
> + tree_code def_stmt_code = gimple_assign_rhs_code (def_stmt);
> + if ((def_stmt_code == EQ_EXPR || def_stmt_code == NE_EXPR)
> + && (calc_stmts.find (def_stmt) == calc_stmts.end ())
> + && (stmts_to_fold.find (def_stmt) == stmts_to_fold.end ()))
2 redundant pairs of ()s.
Jakub
