We sent a new version: https://gcc.gnu.org/pipermail/gcc-patches/2026-May/718178.html.
On Sun, May 3, 2026 at 7:24 PM Jeffrey Law <[email protected]> wrote: > > > On 3/26/2026 4:37 AM, Konstantinos Eleftheriou wrote: > > Ping for https://gcc.gnu.org/pipermail/gcc-patches/2025-June/687530.html > . > Sorry this is taking so long to get resolved. I've reviewed the > comments on the prior variants to get context. > > So in find_terminal_nodes you have this: > > > + if (TREE_CODE (expr) != SSA_NAME) > > + { > > + terminal_nodes->add (expr); > > + return; > > + } > > + > > > That implies things other than SSA_NAMEs get onto the terminal node list > for trees. Then in the comparison function: > > +template<> > > +int sort_elements<tree> (const void *p1, const void *p2) > > +{ > > + const tree t1 = *(const tree *)p1; > > + const tree t2 = *(const tree *)p2; > > + > > + return SSA_NAME_VERSION (t1) - SSA_NAME_VERSION (t2); > > +} > > You're testing SSA_NAME_VERSION, which obviously only exists on > SSA_NAMEs. So unless there's a step where non-SSA_NAME objects get > filtered, something seems wrong here. You're right that `find_terminal_nodes` can put non-SSA_NAMEs onto a terminal-node set. The reason it's safe is that those sets are never sorted; `sort_elements` is only ever applied to `expr_terms` and `related_terms`, whose members are the comparison operands and are always SSA_NAMEs. To make that precondition explicit (and catch any future misuse), v5 adds an assert in the comparator. > > > + tree def_lhs = gimple_assign_lhs (def_stmt); > > + tree def_op1 = gimple_assign_rhs1 (def_stmt); > > + tree def_op2 = gimple_assign_rhs2 (def_stmt); > > + tree def_op3 = gimple_assign_rhs3 (def_stmt); > Do we always have 3 RHS operands? That (and instance about 15 lines > later) look suspicious as I immediately see anything that would > guarantee that we have 3 source operands on these statements. > In every successful match both statements are two-operand comparisons (EQ/NE, or XOR treated as NE), so gimple_assign_rhs3 was always NULL. The rhs3 reads and the comparison were dead, so v5 removes them entirely. Generally it looks sane and I'm inclined to ACK once the concerns above > are resolved one way or another. > > jeff > > Thanks, Konstantinos
