Hi Jakub, thanks for the feedback.
We have sent a new version
(https://gcc.gnu.org/pipermail/gcc-patches/2025-June/687530.html),
addressing those issues. Regarding the hash_sets, we have replaced
them with vectors in some cases and in the cases that we're still
using them we're copying them to sorted vectors before traversals.
Konstantinos
On Thu, Jun 12, 2025 at 10:36 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> 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
>
