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)
