On Mon, Mar 17, 2025 at 11:40:32AM +0100, Konstantinos Eleftheriou wrote:
> * gcc.dg/tree-ssa/fold-xor-and-or.c:
> Remove logical-op-non-short-circuit=1.
The remove certainly fits on the same line as :
and --param= is missing before the option name.
> +/* { dg-final { scan-tree-dump-not "d1_\[0-9\]+.D. \\^ d2_\[0-9\]+.D."
> "optimized" } } */
> \ No newline at end of file
Please avoid files not ending with newline unless intentional.
> +/* { dg-final { scan-tree-dump-not "d1_\[0-9\]+.D. \\^ d2_\[0-9\]+.D."
> "optimized" } } */
> \ No newline at end of file
Ditto.
> --- a/gcc/tree-ssa-reassoc.cc
> +++ b/gcc/tree-ssa-reassoc.cc
> @@ -4077,6 +4077,359 @@ 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
Missing static before tree
> +solve_expr (tree expr, hash_set<gimple *> *calc_stmts,
> + hash_set<gimple *> *stmts_to_fold, hash_set<tree> *visited,
> + bool is_or_expr)
> +{
> + /* Return, if have already visited this expression or the expression is not
> + an SSA name. */
> + if (visited->contains (expr) || TREE_CODE (expr) != SSA_NAME)
The TREE_CODE (expr) != SSA_NAME test is certainly much cheaper than
visited->contains (expr), so please swap the || operands.
+void
Again missing static before return type (and in more spots)
+find_terminal_nodes (tree expr, hash_set<tree> *terminal_nodes,
+ hash_set<tree> *visited)
+{
+ if (visited->contains (expr))
+ return;
+
+ visited->add (expr);
The above together is
if (visited->add (expr))
return;
(and more efficient in that).
> + return NULL_TREE;
> +
> + visited->add (expr);
> +
> + gimple *def_stmt = SSA_NAME_DEF_STMT (expr);
> +
> + if (!def_stmt || !is_gimple_assign (def_stmt))
> + return expr;
> +
> + unsigned int op_num = gimple_num_ops (def_stmt);
> + unsigned int terminal_node_num = 0;
> + /* Visit the expression operands recursively until finding a statement that
until it finds ?
> +
> + do {
The formatting is wrong, it shouldn't be
do {
statements;
} while (cond);
but
do
{
statements;
}
while (cond);
Last but not least, there are ton's of hash_sets involved, wonder if one could
away without that for the common simple cases and use those only if it is
larger, but more importantly, I believe the code generation depends on the
hash_set traversals, which is a big no no for reproduceability, because the
hash_set<tree> or hash_set<gimple *> I believe just use hashes derived from
the pointer values and so with address space randomization, even subsequent
runs of the same compiler on the same machine could result in different code
generation, not even talking about cross compilers with different hosts etc.
It is fine to use hash set traversals to find out what will need to be done,
but in that case it should be e.g. pushed into some vector worklist and the
worklist sorted by something stable (e.g. SSA_NAME_VERSIONs or positions in
the original term sequence etc., i.e. something that reflects the IL and
not the pointer values of particular trees or gimple *).
Jakub
