On Mon, 9 Jun 2025, Jakub Jelinek wrote:
> On Sun, Jun 08, 2025 at 10:49:44AM +0200, Richard Biener wrote:
> > I'm also a bit nervous about this given during RTL expansion the IL is
> > neither fully GIMPLE nor fully RTL. Given we do not perform many
> > range queries we might be just lucky to not run into any issues?
>
> So, I've added following incremental patch:
>
> --- gcc/expr.cc.jj 2025-06-09 15:07:38.882265627 +0200
> +++ gcc/expr.cc 2025-06-09 16:18:26.988947027 +0200
> @@ -66,6 +66,7 @@ along with GCC; see the file COPYING3.
> #include "tree-pretty-print.h"
> #include "flags.h"
> #include "internal-fn.h"
> +#include "value-query.h"
>
>
> /* If this is nonzero, we do not bother generating VOLATILE
> @@ -11348,6 +11348,25 @@ expand_expr_real_1 (tree exp, rtx target
> LAST_VIRTUAL_REGISTER + 1);
> }
>
> + {
> + gimple *cg = currently_expanding_gimple_stmt;
> + if (INTEGRAL_TYPE_P (TREE_TYPE (exp)))
> + {
> + int_range_max vr;
> + get_range_query (cfun)->range_of_expr (vr, exp, cg);
> + }
> + else if (SCALAR_FLOAT_TYPE_P (TREE_TYPE (exp)))
> + {
> + frange vr;
> + get_range_query (cfun)->range_of_expr (vr, exp, cg);
> + }
> + else if (POINTER_TYPE_P (TREE_TYPE (exp)))
> + {
> + prange vr;
> + get_range_query (cfun)->range_of_expr (vr, exp, cg);
> + }
> + }
> +
> g = get_gimple_for_ssa_name (exp);
> /* For EXPAND_INITIALIZER try harder to get something simpler. */
> if (g == NULL
>
> (in first attempt this was guarded with if (flag_checking), but that
> was a bad idea, because it then causes bootstrap comparison failure,
> one stage built with -fno-checking, another with -fchecking and the
> different range queries result in different answers from the other range
> queries). I don't think we want to guard that with #if CHECKING_P either
> though, if it can cause different code generation (I think there were about
> 15 .bad_compare files on x86_64 and none on i686), then we'd be testing for
> months something that we'll actually not release afterwards.
>
> And the only issue it found (in x86_64 and i686-linux bootstraps/regtests)
> is that the 4 remove_edge calls done during expansion will not remove
> phi arguments and so ranger can then ICE.
>
> With the following incremental patch I don't see other problems.
OK, that's re-assuring.
When we remove edges we change the CFG - does the ranger cache
need any updating for this?
> --- gcc/cfgexpand.h.jj 2025-04-08 14:08:48.481320427 +0200
> +++ gcc/cfgexpand.h 2025-06-09 15:01:52.946766978 +0200
> @@ -22,6 +22,7 @@ along with GCC; see the file COPYING3.
>
> extern tree gimple_assign_rhs_to_tree (gimple *);
> extern HOST_WIDE_INT estimated_stack_frame_size (struct cgraph_node *);
> +extern void expand_remove_edge (edge);
> extern void set_parm_rtl (tree, rtx);
>
>
> --- gcc/cfgexpand.cc.jj 2025-06-09 14:18:56.000000000 +0200
> +++ gcc/cfgexpand.cc 2025-06-09 15:01:36.423981975 +0200
> @@ -2818,6 +2818,19 @@ label_rtx_for_bb (basic_block bb)
> }
>
>
> +/* Wrapper around remove_edge during expansion. */
> +
> +void
> +expand_remove_edge (edge e)
> +{
> + if (current_ir_type () != IR_GIMPLE
> + && (e->dest->flags & BB_RTL) == 0
> + && !gimple_seq_empty_p (phi_nodes (e->dest)))
> + remove_phi_args (e);
> + remove_edge (e);
> +}
> +
> +
> /* A subroutine of expand_gimple_cond. Given E, a fallthrough edge
> of a basic block where we just expanded the conditional at the end,
> possibly clean up the CFG and instruction sequence. LAST is the
> @@ -2840,7 +2853,7 @@ maybe_cleanup_end_of_block (edge e, rtx_
> if (BARRIER_P (get_last_insn ()))
> {
> rtx_insn *insn;
> - remove_edge (e);
> + expand_remove_edge (e);
> /* Now, we have a single successor block, if we have insns to
> insert on the remaining edge we potentially will insert
> it at the end of this block (if the dest block isn't feasible)
> @@ -4460,7 +4473,7 @@ expand_gimple_tailcall (basic_block bb,
> if (e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
> e->dest->count -= e->count ();
> probability += e->probability;
> - remove_edge (e);
> + expand_remove_edge (e);
> }
> else
> ei_next (&ei);
> @@ -7311,7 +7324,7 @@ pass_expand::execute (function *fun)
> In the future we should get this fixed properly. */
> if ((e->flags & EDGE_ABNORMAL)
> && !(e->flags & EDGE_SIBCALL))
> - remove_edge (e);
> + expand_remove_edge (e);
> else
> ei_next (&ei);
> }
> --- gcc/stmt.cc.jj 2025-04-08 14:09:00.253156547 +0200
> +++ gcc/stmt.cc 2025-06-09 15:18:30.714708828 +0200
> @@ -52,6 +52,7 @@ along with GCC; see the file COPYING3.
> #include "tree-cfg.h"
> #include "dumpfile.h"
> #include "builtins.h"
> +#include "cfgexpand.h"
>
>
> /* Functions and data structures for expanding case statements. */
> @@ -1025,7 +1026,7 @@ expand_case (gswitch *stmt)
> && gimple_seq_unreachable_p (bb_seq (default_edge->dest)))
> {
> default_label = NULL;
> - remove_edge (default_edge);
> + expand_remove_edge (default_edge);
> default_edge = NULL;
> }
>
>
> Another option would be to add to the
> IR_GIMPLE,
> IR_RTL_CFGRTL,
> IR_RTL_CFGLAYOUT
> cases add IR_EXPAND which would be something in between IR_GIMPLE
> and IR_RTL_CFGRTL, the IL is mostly GIMPLE, but new basic blocks
> are created as BB_RTL and stuff like edge removal would check if
> e->dest->flags & BB_RTL or not. And e.g. edge insertions are RTL
> instead of GIMPLE. Perhaps cleaner but might be too much work.
Yeah. Likewise the idea I had on IRC to expand to a new CFG and
keep the GIMPLE CFG unmodified. You'd have to pass "the cfg" around
everywhere (instead of the function via cfun).
> > Can we maybe try forcing a range query for all SSA names we expand
> > to increase coverage? I'll also note that
> >
> > get_global_range_query ()->range_of_expr (arg1, arg2)
> >
> > does not use any context, but only context for defs when querying up
> > the chain. Context should generally be the current stmt we expand,
>
> Sure, that is why the patch replaces those with
> !get_range_query (cfun)->range_of_expr (\arg1, arg2, stmt)
> where stmt is in the end usually currently_expanding_gimple_stmt.
Ah, OK. I obviously did not look at most of the actual patch yet.
> > That said, I'd be a little less nervous if we'd process basic-blocks
> > in dominator order instead of in the way we do now. IIRC we do not
> > create new basic-blocks up to the first expand_gimple_basic_block?
>
> We do create them. E.g. the init_block (new successor of ENTRY bb).
> There are others created during the expand_gimple_basic_block calls.
> And something perhaps changeable but not changed by me is that
> ENTRY and EXIT are changed to BB_RTL early as well (but they don't
> contain instructions, so it isn't that big deal).
>
> > So using, say, get_all_dominated_blocks (we free dominators, possibly
> > PHI expansion might insert on edges) to get a PRE ordered set of
>
> I think PHI expansion does insert on edges.
Yes, it must (but we expand those separately, so we could tear down
ranger then).
So apart from ICEing I wonder how ranger (and it's cache) deals with
basic blocks being added, edges vanishing (we kill off all abnormal
edges - those ranger usually "stops" at, for correctness), edges
being added (find_many_sub_basic_blocks), etc. - are we sure we
get correct range answers when the cache was from a "different"
CFG (the set of CFG manipulations are of course constrained, but
this isn't very well documented and also not very well ordered/isolated)
Richard.
> > blocks to visit, just to make it less random.
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
