On Mon, 3 Nov 2025, Andrew MacLeod wrote: > > On 11/3/25 13:24, 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? > > > > > > 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 > > > FWIW, The patch does bootstrap and regression testing. It could be tweaked.. > the while loop shouldn't be needed... there should only ever be one sentinel.
Yes please - also the first use can never be a sentinel so you only need to hande it in next_readonly_imm_use. And I'd do end_readonly_imm_use_p first and adjust iter_node.next only when checking. > Do we actually have a situation where we have nested iterators? wouldn't the > inner one trap on the sentinel for the outer iterator, the same way the FAST > iterator was trapping in the PR? > > FOR_EACH_IMM_USE_FAST was originally intended to literally be a read only > version... It is not meant to support changing the operand, just looking. > If something might be changed, used the other one. And It was never intended > to be nested with another one, although it should be harmless and I see no > reason not to support it > > The other version was intended to allow for deletions and rewriting of > operands at the statement level... on a single statement at a time. if > there are *additions* made to the list during an iteration, the results will > be undefined. The new addition may or may not be visited. I don't think > there is much that can be done about that.. if you cached all the statements > before iterating, you wouldn't get the new addition either. > > Changing operands on a different statement for an SSA_NAME while iterating is > also going to be undefined behaviour and wouldnt be supported either. If we > kept the sentinel, maybe we could add a checking condition that when we add a > new statement and link in the operands, we scan the list and make sure there > isnt an active sentinel in the list. > > 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)
