On 11/3/25 09:43, Richard Biener wrote:
The following implements a prototype for additional checking around
SSA immediate use iteration. Specifically this guards immediate
use list modifications inside a FOR_EACH_IMM_USE_STMT iteration
to be restricted to uses involving the actual stmt. It likewise
prohibits FOR_EACH_IMM_USE_STMT from inside another such iteration.
Checking is performend at FOR_EACH_IMM_USE_STMT start and the
iteration tracks the actual stmt being iterated in the 'use'
field of the SSA name immediate use field, enforcing that in
delink_imm_use (currently, as the main guard against removing
the immediate use following the current stmt set).
I haven't had to think about the immediate use code in a long time...
I was poking friday at a depth counter to avoid nested calls other than
FAST, but you've been delving deeper than that obviously.
FOR_EACH_IMM_USE_FAST is out of scope for the moment, we'd need
more space for state. Likewise this is a prototype as it re-uses
the SSA name immediate use nodes 'use' field and thus had to disable
checking around the immediate use health. For FOR_EACH_IMM_USE_FAST
I'd like to have a flag indicating a fast iteration is taking place
(which necessarily would have to be a 'depth'). Starting a
FOR_EACH_IMM_USE_STMT should only be with depth == 0. And with
depth != 0 we do not want to change the immediate use list.
The patch depends on the previous one removing the fake sentinel
immediate use entry. The patch triggers issues elsewhere, like
for gcc.dg/torture/20181029-1.c, see 2/3 or during bootstrap,
see 3/3. The series does not bootstrap (more issues).
What this basically shows is that while the sentinel used with
FOR_EACH_IMM_USE_STMT ensures iteration itself doesn't break,
we do have cases where we disrupt this iteration in a way that
can cause stmts to be missed in the iteration (or selective uses
on one stmt for a nested FOR_EACH_IMM_USE_ON_STMT). And that
can be quite unexpected as the forwprop case shows (one of the
remaining update_stmt in that loop is still a problem).
Yeah, the sentinels main goal was to ensure the iteration works when
things were removed.
Most definitely the dependent patch removing the sentinel will
eventually cause iteration to "break", either running off list
and crashing or re-iterating parts of the uses. But I also think
the current situation isn't sustainable (and the original issue
with ranger still exists). Also FOR_EACH_IMM_USE_FAST can be
similarly affected in case we go off to code updating a random
other stmt - like in forwprop if you release an unrelated
SSA name it might insert a debug stmt and update all of that
names uses and its use stmts, if that also has a use of the
fast iteration SSA name and this happens to be the one that
is queued as "next" then you'll visit some uses twice due
to update_stmt removing & re-inserting SSA uses. A use
might also vanish completely (and crash the iteration) if
downthread a stmt with a use is folded.
I'm not sure what the take away is here - immediate uses are bad?
I think the main issue is we never define what is safe and what is
unsafe while iterating.
I also think it is reasonable to have checks as much as we can.
I think the FAST iterator can be made always safe, even if an unsafe one
is already iterating I don't see why it couldn't identify sentinels
and skip them.. the USE field should never be NULL except for a
sentinel or the start node. The attached patch does that and appears
to fix the original failure.. but it does not tackle any of the more
complex situations. Im testing it with a bootstrap now
Well, "always safe" as long as an unsafe iteration doesn't occur during
the fast one processing :-P I think that should simply be not
allowed. Definition of the FAST version is that nothing changes... if
we call code that breaks that rule, well, you shouldn't be using the
fast version. Its impossible to make it work properly.
I'll continue thinking about nested interactions of the regular
version.... I do not think it was ever planned/expected to have nested
iteration loops that can change the IL. Im not sure that should be
allowed either. It is very difficult to define what the expected
behaviour in the outer loop is.. In cases like that, we probably need a
more expensive version that pre-caches everything.. otherwise the outer
iteration would be difficult to define expectations.
Andrew
From 24757805a6b9311f470426ac097b2c450bf56827 Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <[email protected]>
Date: Mon, 3 Nov 2025 12:50:17 -0500
Subject: [PATCH] make FOR_EACH_IMM_USE_FAST ignore sentinel nodes.
---
gcc/ssa-iterators.h | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/gcc/ssa-iterators.h b/gcc/ssa-iterators.h
index 0822a98323d..7682e113d51 100644
--- a/gcc/ssa-iterators.h
+++ b/gcc/ssa-iterators.h
@@ -345,6 +345,11 @@ first_readonly_imm_use (imm_use_iterator *imm, tree var)
{
imm->end_p = &(SSA_NAME_IMM_USE_NODE (var));
imm->imm_use = imm->end_p->next;
+
+ // Skip any sentinel markers. Identified with a NULL use.
+ while (!imm->imm_use->use && !end_readonly_imm_use_p (imm))
+ imm->imm_use = imm->imm_use->next;
+
imm->iter_node.next = imm->imm_use->next;
if (end_readonly_imm_use_p (imm))
return NULL_USE_OPERAND_P;
@@ -362,12 +367,15 @@ next_readonly_imm_use (imm_use_iterator *imm)
via stmt changes, or SET_USE, or somesuch thing, and you need to be
using the SAFE version of the iterator. */
if (flag_checking)
- {
- gcc_assert (imm->iter_node.next == old->next);
- imm->iter_node.next = old->next->next;
- }
+ gcc_assert (imm->iter_node.next == old->next);
imm->imm_use = old->next;
+
+ // Skip any sentinel markers. Identified with a NULL use.
+ while (!imm->imm_use->use && !end_readonly_imm_use_p (imm))
+ imm->imm_use = imm->imm_use->next;
+ imm->iter_node.next = imm->imm_use->next;
+
if (end_readonly_imm_use_p (imm))
return NULL_USE_OPERAND_P;
return imm->imm_use;
--
2.45.0
