Fix recursive inline member ordering in optimizer - #20111
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This reverts commit a942862.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
This reverts commit 7bfd44a.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
It works. I'll mark as ready for review but I'm not sure the approach here is the best. |
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
| List.mapFold (OptimizeBinding cenv isRec) env xs | ||
| if isRec then | ||
| let xsArray = xs |> List.toArray | ||
| let order = GetBindingOptimizationOrder cenv false true xs |
There was a problem hiding this comment.
Unlike the module-level path (OptimizeModuleBindings, which gates on binds |> List.exists (fun b -> b.Var.ShouldInline)), this local recursive path runs the full dependency analysis unconditionally for every let rec group — including groups with no inline bindings. That adds a double expression traversal (freeInExpr + FoldExpr, plus trait-witness codegen) on a hot compile path, and it changes emitted IL for non-inline recursive functions (see the updated Verify13043 and Regression_TLR_MutualInnerRec_* baselines), which contradicts the PR statement that non-inline groups retain existing behavior. Consider gating this branch on the same inline-presence check and falling back to the original List.mapFold (OptimizeBinding cenv isRec) otherwise.
| | Expr.Op(TOp.TraitCall traitInfo, _, args, m) -> | ||
| let depIdxs = addTraitSolutionDependencies depIdxs traitInfo | ||
|
|
||
| match ConstraintSolver.CodegenWitnessExprForTraitConstraint cenv.TcVal cenv.g cenv.amap m traitInfo args with |
There was a problem hiding this comment.
Witness-codegen failures here are silently swallowed (| _ -> depIdxs). Any sibling dependency that is reachable only through the synthesized witness (the resolved member) is then dropped from the schedule. If witness resolution is itself order-sensitive and fails while a sibling is still unoptimized, the ordering problem this PR fixes could reappear for trait-witness cases. Worth confirming the error branch can never hide a real sibling dependency (and note this re-runs witness codegen purely for dependency discovery, which is not free).
| let visiting = HashSet<int>() | ||
| let visited = HashSet<int>() | ||
|
|
||
| let rec visit idx = |
There was a problem hiding this comment.
visit and the addBindingDependencies/FoldExpr walk are not stack-guarded, unlike other expression traversals in this file (CollectLocalsWithStackGuard, StackGuard). Deeply nested expressions or very large recursive groups could overflow the stack. Consider routing these through the existing stack-guard mechanism.
| let fvs = freeInExpr CollectLocalsNoCaching expr | ||
|
|
||
| let depIdxs = | ||
| let depIdxs = addVals depIdxs (fvs.FreeLocals |> Zset.elements) |
There was a problem hiding this comment.
freeInExpr already collects FreeLocals, and the FoldExpr below re-adds the same locals via its Expr.Val intercept — the two passes largely duplicate work for every binding. The fold is only strictly needed for the TraitCall witness handling; you could take the plain value dependencies from freeInExpr alone and reserve the fold for trait calls, avoiding a second full traversal per binding.
Recursive inline members in a recursive binding group can depend on siblings that appear later in source order. The optimizer now discovers these dependencies, optimizes bindings in dependency order, and restores source order before emitting the result. Trait-witness dependencies and module-level recursive groups are handled as well, while non-inline recursive groups retain the existing behavior.
Regression coverage exercises recursive inline member access, trait-witness resolution, and the existing Issue 1565 cases. Emitted-IL and AOT baselines are updated for the resulting stable output.
Fixes #1565