-
Notifications
You must be signed in to change notification settings - Fork 870
Fix recursive inline member ordering in optimizer #20111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6c25d66
54b5fbe
3f5f34c
fd22df9
f7e7e9e
a942862
d4dc94b
78b0011
0d8eceb
66fee1d
1a09b70
79ce76a
d1b02eb
492a850
7a1f5da
7eed09a
5d57c2e
23213a0
f1c0776
00f9f28
7bfd44a
7cddf24
65a188e
1673a1f
0f4d469
f41fc0e
e941879
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -633,7 +633,7 @@ let GetInfoForLocalValue cenv env (v: Val) m = | |
| match env.localExternalVals.TryFind v.Stamp with | ||
| | Some vval -> vval | ||
| | None -> | ||
| if v.ShouldInline then | ||
| if cenv.optimizing && v.ShouldInline then | ||
| errorR(Error(FSComp.SR.optValueMarkedInlineButWasNotBoundInTheOptEnv(fullDisplayTextOfValRef (mkLocalValRef v)), m)) | ||
| UnknownValInfo | ||
|
|
||
|
|
@@ -3191,11 +3191,11 @@ and TryOptimizeVal cenv env (vOpt: ValRef option, shouldInline, inlineIfLambda, | |
| | TupleValue _ | UnionCaseValue _ | RecdValue _ when shouldInline -> | ||
| failwith "tuple, union and record values cannot be marked 'inline'" | ||
|
|
||
| | UnknownValue when shouldInline && cenv.settings.alwaysInline -> | ||
| | UnknownValue when shouldInline && cenv.settings.alwaysInline && cenv.optimizing -> | ||
| warning(Error(FSComp.SR.optValueMarkedInlineHasUnexpectedValue(), m)) | ||
| None | ||
|
|
||
| | _ when shouldInline && cenv.settings.alwaysInline -> | ||
| | _ when shouldInline && cenv.settings.alwaysInline && cenv.optimizing -> | ||
| warning(Error(FSComp.SR.optValueMarkedInlineCouldNotBeInlined(), m)) | ||
| None | ||
|
|
||
|
|
@@ -3241,7 +3241,7 @@ and OptimizeVal cenv env expr (v: ValRef, m) = | |
| e, AddValEqualityInfo g m v einfo | ||
|
|
||
| | None -> | ||
| if cenv.settings.alwaysInline then | ||
| if cenv.optimizing && cenv.settings.alwaysInline then | ||
| if v.ShouldInline then | ||
| match valInfoForVal.ValExprInfo with | ||
| | UnknownValue -> error(Error(FSComp.SR.optFailedToInlineValue(v.DisplayName), m)) | ||
|
|
@@ -4491,7 +4491,20 @@ and OptimizeBinding cenv isRec env (TBind(vref, expr, spBind)) = | |
| raise (ReportedError (Some exn)) | ||
|
|
||
| and OptimizeBindings cenv isRec env xs = | ||
| List.mapFold (OptimizeBinding cenv isRec) env xs | ||
| if isRec then | ||
| let xsArray = xs |> List.toArray | ||
| let order = GetBindingOptimizationOrder cenv false true xs | ||
|
|
||
| let results, env = | ||
| (env, order) | ||
| ||> List.mapFold (fun env idx -> | ||
| let result, env = OptimizeBinding cenv isRec env xsArray[idx] | ||
| (idx, result), env) | ||
|
|
||
| let resultsByIndex = results |> Map.ofList | ||
| [ for idx in 0 .. xsArray.Length - 1 -> resultsByIndex[idx] ], env | ||
| else | ||
| List.mapFold (OptimizeBinding cenv isRec) env xs | ||
|
|
||
| and OptimizeModuleExprWithSig cenv env mty def = | ||
| let g = cenv.g | ||
|
|
@@ -4581,11 +4594,109 @@ and OptimizeModuleExprWithSig cenv env mty def = | |
| and mkValBind (bind: Binding) info = | ||
| (mkLocalValRef bind.Var, info) | ||
|
|
||
| and GetBindingOptimizationOrder cenv inlineDependenciesOnly preferLowArity (binds: Binding list) = | ||
| // Recursive binding groups are published to the optimizer incrementally as each binding is | ||
| // processed. If a caller is optimized before a later sibling it depends on, inline lookup can | ||
| // observe an incomplete optimization environment. Compute a dependency-first schedule for the | ||
| // recursive group, then restore source order after optimization. | ||
| let bindsArray = binds |> List.toArray | ||
|
|
||
| let bindIndexByStamp = | ||
| binds | ||
| |> List.mapi (fun idx bind -> bind.Var.Stamp, idx) | ||
| |> Map.ofList | ||
|
|
||
| let addDependency depIdxs stamp = | ||
| match bindIndexByStamp |> Map.tryFind stamp with | ||
| | Some depIdx when not inlineDependenciesOnly || bindsArray[depIdx].Var.ShouldInline -> | ||
| Set.add depIdx depIdxs | ||
| | None -> depIdxs | ||
| | Some _ -> depIdxs | ||
|
|
||
| let rec addBindingDependencies depIdxs expr = | ||
| let addVals depIdxs vals = | ||
| vals | ||
| |> Seq.fold (fun depIdxs (v: Val) -> addDependency depIdxs v.Stamp) depIdxs | ||
|
|
||
| let rec addTraitSolutionDependencies depIdxs (traitInfo: TraitConstraintInfo) = | ||
| match traitInfo.Solution with | ||
| | Some(FSMethSln(_, vref, _, _)) -> addDependency depIdxs vref.Deref.Stamp | ||
| | Some(ClosedExprSln witnessExpr) -> addBindingDependencies depIdxs witnessExpr | ||
| | _ -> depIdxs | ||
|
|
||
| let fvs = freeInExpr CollectLocalsNoCaching expr | ||
|
|
||
| let depIdxs = | ||
| let depIdxs = addVals depIdxs (fvs.FreeLocals |> Zset.elements) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| addVals depIdxs (fvs.FreeTyvars.FreeTraitSolutions |> Zset.elements) | ||
|
|
||
| let folder = | ||
| { ExprFolder0 with | ||
| exprIntercept = | ||
| (fun _exprF noInterceptF depIdxs expr -> | ||
| let depIdxs = | ||
| match expr with | ||
| | Expr.Val(vref, _, _) -> addDependency depIdxs vref.Deref.Stamp | ||
| // Member-constraint calls can hide the real sibling dependency behind | ||
| // a witness expression, so fold over the resolved witness as well. | ||
| | 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Witness-codegen failures here are silently swallowed ( |
||
| | OkResult (_, Some witnessExpr) -> addBindingDependencies depIdxs witnessExpr | ||
| | _ -> depIdxs | ||
| | _ -> depIdxs | ||
|
|
||
| noInterceptF depIdxs expr) } | ||
|
|
||
| FoldExpr folder depIdxs expr | ||
|
|
||
| let dependencyIndexes = | ||
| binds | ||
| |> List.map (fun (TBind(_, expr, _)) -> | ||
| addBindingDependencies Set.empty expr |> Set.toArray) | ||
| |> List.toArray | ||
|
|
||
| let ordered = ResizeArray() | ||
| let visiting = HashSet<int>() | ||
| let visited = HashSet<int>() | ||
|
|
||
| let rec visit idx = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| if not (visited.Contains idx) then | ||
| if not (visiting.Contains idx) then | ||
| visiting.Add idx |> ignore | ||
|
|
||
| for depIdx in dependencyIndexes[idx] do | ||
| if depIdx <> idx then | ||
| visit depIdx | ||
|
|
||
| visiting.Remove idx |> ignore | ||
| visited.Add idx |> ignore | ||
| ordered.Add idx | ||
|
|
||
| let rootOrder = | ||
| [ 0 .. binds.Length - 1 ] | ||
| |> (if preferLowArity then | ||
| List.sortBy (fun idx -> | ||
| let arity = | ||
| bindsArray[idx].Var.ValReprInfo | ||
| |> Option.map (fun repr -> repr.TotalArgCount) | ||
| |> Option.defaultValue 0 | ||
|
|
||
| arity, -idx) | ||
| else | ||
| id) | ||
|
|
||
| for idx in rootOrder do | ||
| visit idx | ||
|
|
||
| ordered |> Seq.toList | ||
|
|
||
| and OptimizeModuleContents cenv (env, bindInfosColl) input = | ||
| match input with | ||
| | TMDefRec(isRec, opens, tycons, mbinds, m) -> | ||
| let env = if isRec then BindInternalValsToUnknown cenv (allValsOfModDef input) env else env | ||
| let mbindInfos, (env, bindInfosColl) = OptimizeModuleBindings cenv (env, bindInfosColl) mbinds | ||
| let mbindInfos, (env, bindInfosColl) = OptimizeModuleBindings cenv isRec (env, bindInfosColl) mbinds | ||
| let mbinds, minfos = List.unzip mbindInfos | ||
| let binds = minfos |> List.choose (function Choice1Of2 (x, _) -> Some x | _ -> None) | ||
| let binfos = minfos |> List.choose (function Choice1Of2 (_, x) -> Some x | _ -> None) | ||
|
|
@@ -4615,8 +4726,35 @@ and OptimizeModuleContents cenv (env, bindInfosColl) input = | |
| let (defs, info), (env, bindInfosColl) = OptimizeModuleDefs cenv (env, bindInfosColl) defs | ||
| (TMDefs defs, info), (env, bindInfosColl) | ||
|
|
||
| and OptimizeModuleBindings cenv (env, bindInfosColl) xs = | ||
| List.mapFold (OptimizeModuleBinding cenv) (env, bindInfosColl) xs | ||
| and OptimizeModuleBindings cenv isRec (env, bindInfosColl) xs = | ||
| let bindingGroup = | ||
| xs | ||
| |> List.map (function | ||
| | ModuleOrNamespaceBinding.Binding bind -> Some bind | ||
| | _ -> None) | ||
|
|
||
| let binds = bindingGroup |> List.choose id | ||
|
|
||
| if | ||
| isRec | ||
| && (bindingGroup |> List.forall Option.isSome) | ||
| && (binds |> List.exists (fun bind -> bind.Var.ShouldInline)) | ||
| then | ||
| let xsArray = xs |> List.toArray | ||
| let preferLowArity = binds |> List.forall (fun bind -> bind.Var.IsMember) | ||
| let order = GetBindingOptimizationOrder cenv true preferLowArity binds | ||
|
|
||
| let results, (env, bindInfosColl) = | ||
| ((env, bindInfosColl), order) | ||
| ||> List.mapFold (fun state idx -> | ||
| let result, state = OptimizeModuleBinding cenv state xsArray[idx] | ||
| (idx, result), state) | ||
|
|
||
| let resultsByIndex = results |> Map.ofList | ||
| // Keep the emitted binding list in source order; only the optimization schedule changes. | ||
| [ for idx in 0 .. xsArray.Length - 1 -> resultsByIndex[idx] ], (env, bindInfosColl) | ||
| else | ||
| List.mapFold (OptimizeModuleBinding cenv) (env, bindInfosColl) xs | ||
|
|
||
| and OptimizeModuleBinding cenv (env, bindInfosColl) x = | ||
| match x with | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| namespace EmittedIL.Inlining | ||
|
|
||
| open Xunit | ||
| open FSharp.Test | ||
| open FSharp.Test.Compiler | ||
|
|
||
| module Regression_RecursiveInlineMemberDependencies = | ||
|
|
||
| let private assertCompiles source = | ||
| source | ||
| |> withOptimize | ||
| |> compile | ||
| |> shouldSucceed | ||
| |> ignore | ||
|
|
||
| [<Fact>] | ||
| let ``Inline members that depend on sibling member access compile`` () = | ||
| FSharp """ | ||
| module MemberAccessDependencyRepro | ||
|
|
||
| type ValidationBuilder() = | ||
| member inline _.Return(value: int) : int = value | ||
|
|
||
| member inline this.Bind(value: int, binder: int -> int) : int = | ||
| let result = this.Source value | ||
| binder result | ||
|
|
||
| member inline this.Source(value: int) : int = value | ||
|
|
||
| let inline run (builder: ValidationBuilder) = | ||
| builder.Bind(1, fun x -> x + 1) | ||
| """ | ||
| |> assertCompiles | ||
|
|
||
| [<Fact>] | ||
| let ``Trait-witness inline overload consumers compile`` () = | ||
| FSharp """ | ||
| module TraitWitnessOverloadRepro | ||
|
|
||
| open System.Runtime.InteropServices | ||
|
|
||
| type Default1 = class end | ||
|
|
||
| type Intersperse = | ||
| inherit Default1 | ||
|
|
||
| static member inline Intersperse (x: '``Collection<'T>``, e: 'T, [<Optional>]_impl: Default1) = | ||
| x | ||
|
|
||
| static member Intersperse (x: list<'T>, e: 'T, [<Optional>]_impl: Intersperse) = | ||
| x | ||
|
|
||
| static member inline Invoke (sep: 'T) (source: '``Collection<'T>``) = | ||
| let inline call_2 (a: ^a, b: ^b, s) = | ||
| ((^a or ^b): (static member Intersperse: _ * _ * _ -> _) (b, s, a)) | ||
|
|
||
| let inline call (a: 'a, b: 'b, s) = | ||
| call_2 (a, b, s) | ||
|
|
||
| call (Unchecked.defaultof<Intersperse>, source, sep) : '``Collection<'T>`` | ||
|
|
||
| let _ = Intersperse.Invoke 0 [1] | ||
| """ | ||
| |> assertCompiles | ||
|
|
||
| [<Fact>] | ||
| let ``Issue 1565 example 1 compiles`` () = | ||
| FSharp """ | ||
| module Issue1565Example1 | ||
|
|
||
| let inline checkBounds f (g: 'b -> ^c) (tp: ^a) = | ||
| let convertFrom = (^a: (static member name: string) ()) | ||
| let convertTo = (^c: (static member name : string) ()) | ||
| let value = (^a: (member Value: 'b) tp) | ||
|
|
||
| if f value then | ||
| g value | ||
| else | ||
| failwithf "Cannot convert from %s to %s." convertFrom convertTo | ||
|
|
||
| [<Struct>] | ||
| type ConverterA = | ||
| val Value: sbyte | ||
| new(v) = { Value = v } | ||
|
|
||
| static member inline name with get () = "converter-a" | ||
|
|
||
| static member inline convert(x: ConverterA): ConverterB = | ||
| checkBounds ((>=) 0y) (byte >> ConverterB) x | ||
|
|
||
| and [<Struct>] ConverterB = | ||
| val Value: byte | ||
| new(v) = { Value = v } | ||
|
|
||
| static member inline name with get () = "converter-b" | ||
| """ | ||
| |> assertCompiles | ||
|
|
||
| [<Fact>] | ||
| let ``Issue 1565 example 2 compiles`` () = | ||
| FSharp """ | ||
| module Issue1565Example2 | ||
|
|
||
| [<System.Flags>] | ||
| type MyType = | ||
| | Integer = 0b0001 | ||
| | Float = 0b0010 | ||
|
|
||
| module Test = | ||
| [<CustomEquality; NoComparison>] | ||
| type SomeType = | ||
| | Int of int64 | ||
| | Float of float | ||
|
|
||
| override x.Equals other = | ||
| match other with | ||
| | :? SomeType as y -> | ||
| match SomeType.getType x &&& SomeType.getType y with | ||
| | MyType.Integer -> int64 x = int64 y | ||
| | MyType.Float -> float x = float y | ||
| | _ -> false | ||
| | _ -> false | ||
|
|
||
| override x.GetHashCode() = | ||
| match x with | ||
| | Int i -> hash i | ||
| | Float f -> hash f | ||
|
|
||
| static member inline op_Explicit(n: SomeType): float = | ||
| match n with | ||
| | Int i -> float i | ||
| | Float f -> f | ||
|
|
||
| static member inline op_Explicit(n: SomeType): int64 = | ||
| match n with | ||
| | Int i -> i | ||
| | Float f -> int64 f | ||
|
|
||
| static member inline getType x = | ||
| match x with | ||
| | Int _ -> MyType.Integer | ||
| | Float _ -> MyType.Float | ||
| """ | ||
| |> assertCompiles | ||
|
|
||
| [<Fact>] | ||
| let ``Issue 1565 example 3 compiles`` () = | ||
| FSharp """ | ||
| module Test | ||
|
|
||
| type SomeType = | ||
| | Int of int64 | ||
| | Float of float | ||
|
|
||
| static member MyEquals(x, other: SomeType) = | ||
| float x = float other | ||
|
|
||
| static member inline op_Explicit(n: SomeType): float = | ||
| match n with | ||
| | Int i -> float i | ||
| | Float f -> f | ||
|
|
||
| static member inline op_Explicit(n: SomeType): int64 = | ||
| match n with | ||
| | Int i -> i | ||
| | Float f -> int64 f | ||
| """ | ||
| |> assertCompiles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the module-level path (
OptimizeModuleBindings, which gates onbinds |> List.exists (fun b -> b.Var.ShouldInline)), this local recursive path runs the full dependency analysis unconditionally for everylet recgroup — 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 updatedVerify13043andRegression_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 originalList.mapFold (OptimizeBinding cenv isRec)otherwise.