Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: nested-loop defer ordering
Reviewed the full diff for PR #3060 across correctness, performance, security, and documentation accuracy. The fix is well-targeted and correct.
What the change does: when findLoop closes an outer cycle (the pos==0 branch), it now appends any not-yet-finalized block in the same SCC (s.scc == self.scc) before scheduling the cycle's successors. This keeps a nested inner loop body ahead of the enclosing loop's exit, so loop-body defers stay in one contiguous DeferInLoop run and the saved method receiver from #3059 is no longer corrupted by cleanup ordering.
Verified:
- Traced
markInLoop(Tarjan) +findLoopagainst the new"nested body before outer exit"graph; the produced order[0,1,2,4,6,5,3]matches the test and keeps the inner body before the outer exit (block 3). sccis assigned for every block inmarkInLoop(states[w].scc = v), so the zero-value (scc==0) is never a false "unassigned" match — block 0 legitimately owns SCC root 0, and root indices are unique per SCC.- In the
pos==0branch,selfnecessarily shares an SCC withpath[0](there is an edgeself → path[0]andpath[0]reachesself), soself.scccorrectly identifies the closing cycle. - The executable defer-order expectations are all correct against real Go LIFO semantics, including the subtle
breakcase{5,3,1}(breakexits only the inner loop, so the outer loop still processes the second row). - Performance: the two added
O(n)scans run at most once perfindLoop(the branch returns immediately), so they don't change the asymptotic cost. No regression. - Security: no issues — all indices derive from the compiler's own SSA block structure;
fdelis monotonic so no block is appended twice.
Minor (non-blocking) notes — see inline comments:
- Dead field/params in the reproducer (
nestedDeferValue.Type, andoutput'sname/status/args). Fine if intentional (padded struct-by-value + preserved shape), but worth a one-line comment or removal. - A short comment on the
self.scc/path[0]SCC invariant would help the next reader. - Consider a graph test where two or more inner-body blocks remain at cycle close, to pin the multi-member append behavior (current case appends only one block).
No blocking issues found.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
visualfc
approved these changes
Nov 12, 2026
visualfc
left a comment
visualfc
left a comment
Collaborator
There was a problem hiding this comment.
Looks good. Completing the SCC in the pos == 0 branch keeps the inner loop body ahead of the outer exit, so loop defers stay in one DeferInLoop run and the saved method receiver from #3059 stays valid. The new graph order [0,1,2,4,6,5,3] matches a walk of Infos, and the executable LIFO cases (including inner break → {5,3,1}) are right.
One non-blocking coverage nit below.
zhouguangyuan0718
approved these changes
Nov 12, 2026
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A nested loop can leave an inner body unprocessed while scheduling the outer exit. Defers after that exit then enter the compiler cleanup order ahead of the body defers. The saved method receiver in #3059 becomes invalid when cleanup runs.
Schedule the complete strongly connected component before its successors. Add graph ordering coverage and executable
test/goregressions for saved method receivers, LIFO order, return, panic, break and continue. The receiver regression fails with the previous compiler and passes with the fix.Update
defercomplexfor the earlier loop-constant discovery. Its FileCheck identifies loop records through their dispatch header, then checks freeing the node and invoking its saved closure.All 12 CI workflows passed at
79ab9e2659c728751af36feb5230cc380945c92b, with 112 successful checks and 3 conditional skips. Go, native llgo and WASI runtime regressions passed, including the saved receiver, LIFO, panic, break and continue paths. Compiler graph and defercomplex execution/IR checks passed for darwin/arm64 and linux/amd64.Fixes #3059.