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.
--- 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.
> 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.
> 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.
> blocks to visit, just to make it less random.
Jakub
