On Mon, 3 Nov 2025, Andrew MacLeod wrote: > > 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?
Or, in particular, update_stmt is "bad" in how it affects immediate use lists even when nothing changes. > I think the main issue is we never define what is safe and what is unsafe > while iterating. Indeed. > 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 Yes, this might be a good short-term solution. > 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. Yes. This isn't checked right now. I'll probably add a depth field for fast iteration to catch that. Guarded with either ENABLE_GIMPLE_CHECKING or a new ENABLE_IMM_USE_CHECKING, it will enlarge each SSA name by two words :/ > 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. The FAST version has checking that the current node isn't removed, we do not check that nothing else alters the list (like an update_stmt on some random stmt that happens to reference the iterated over SSA name). But with a depth counter such checking could be implemented. > 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. Yes, this shouldn't be allowed (and my patch checks that). I'm going to add a helper that collects use-stmts (without duplicates) into a vector, most FOR_EACH_IMM_USE_STMT users are after that, and the way FOR_EACH_IMM_USE_STMT achieves that is quadratic (due to the imm use list re-sorting). Richard. > > Andrew > > > > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
