From b1f9c724bb9c5b7b8a5512cc127a3cfc99a50b7b Mon Sep 17 00:00:00 2001 From: Don Syme Date: Mon, 23 Aug 2021 23:00:50 +0100 Subject: [PATCH 1/2] fix regression in debug codegen --- src/fsharp/CreateILModule.fs | 2 +- src/fsharp/IlxGen.fs | 11 ++-- src/fsharp/IlxGen.fsi | 2 +- src/fsharp/OptimizeInputs.fs | 2 +- src/fsharp/Optimizer.fs | 39 ++++++------ src/fsharp/Optimizer.fsi | 4 +- src/fsharp/fsi/fsi.fs | 2 +- .../CodeGen/EmittedIL/BooleanLogic.fs | 60 ++++++++++++++++++- 8 files changed, 91 insertions(+), 31 deletions(-) diff --git a/src/fsharp/CreateILModule.fs b/src/fsharp/CreateILModule.fs index 087dcc881e2..1cbb4584175 100644 --- a/src/fsharp/CreateILModule.fs +++ b/src/fsharp/CreateILModule.fs @@ -270,7 +270,7 @@ module MainModuleBuilder = let isDLL = (tcConfig.target = CompilerTarget.Dll || tcConfig.target = CompilerTarget.Module) mkILSimpleModule assemblyName ilModuleName isDLL tcConfig.subsystemVersion tcConfig.useHighEntropyVA ilTypeDefs hashAlg locale flags (mkILExportedTypes exportedTypesList) metadataVersion - let disableJitOptimizations = not (tcConfig.optSettings.jitOpt()) + let disableJitOptimizations = not tcConfig.optSettings.JitOptimizationsEnabled let tcVersion = tcConfig.version.GetVersionInfo(tcConfig.implicitIncludeDir) diff --git a/src/fsharp/IlxGen.fs b/src/fsharp/IlxGen.fs index 974e3457b70..c23b0c8b7c5 100644 --- a/src/fsharp/IlxGen.fs +++ b/src/fsharp/IlxGen.fs @@ -194,7 +194,7 @@ type IlxGenOptions = mainMethodInfo: Attribs option /// Indicates if local optimizations are on - localOptimizationsAreOn: bool + localOptimizationsEnabled: bool /// Indicates if we are generating debug symbols generateDebugSymbols: bool @@ -2099,7 +2099,6 @@ let CodeGenThen cenv mgbuf (entryPointInfo, methodName, eenv, alreadyUsedArgs, c let start = CG.GenerateMark cgbuf "mstart" let innerVals = entryPointInfo |> List.map (fun (v, kind) -> (v, (kind, start))) - (* Call the given code generator *) codeGenFunction cgbuf {eenv with withinSEH=false liveLocals=IntMap.empty() innerVals = innerVals} @@ -5624,7 +5623,7 @@ and GenDecisionTreeSuccess cenv cgbuf inplabOpt stackAtTargets eenv es targetIdx // We have encountered this target before. See if we should generate it now let targetCount = targetCounts.[targetIdx] - let generateTargetNow = isTargetPostponed && cenv.opts.localOptimizationsAreOn && targetCount = 1 && targetNext.Value = targetIdx + let generateTargetNow = isTargetPostponed && cenv.opts.localOptimizationsEnabled && targetCount = 1 && targetNext.Value = targetIdx targetCounts.[targetIdx] <- targetCount - 1 // If not binding anything we can go directly to the targetMarkBeforeBinds point @@ -5683,7 +5682,7 @@ and GenDecisionTreeSuccess cenv cgbuf inplabOpt stackAtTargets eenv es targetIdx // In debug mode, postpone all decision tree targets to after the switching. // In release mode, if a target is the target of multiple incoming success nodes, postpone it to avoid // making any backward branches - let generateTargetNow = cenv.opts.localOptimizationsAreOn && targetCount = 1 && targetNext.Value = targetIdx + let generateTargetNow = cenv.opts.localOptimizationsEnabled && targetCount = 1 && targetNext.Value = targetIdx targetCounts.[targetIdx] <- targetCount - 1 let genTargetInfoOpt = @@ -7083,7 +7082,7 @@ and AllocLocal cenv cgbuf eenv compgen (v, ty, isFixed) (scopeMarks: Mark * Mark let ranges = if compgen then [] else [(v, scopeMarks)] // Get an index for the local let j, realloc = - if cenv.opts.localOptimizationsAreOn then + if cenv.opts.localOptimizationsEnabled then cgbuf.ReallocLocal((fun i (_, ty', isFixed') -> not isFixed' && not isFixed && not (IntMap.mem i eenv.liveLocals) && (ty = ty')), ranges, ty, isFixed) else cgbuf.AllocLocal(ranges, ty, isFixed), false @@ -7152,7 +7151,7 @@ and AllocTopValWithinExpr cenv cgbuf cloc scopeMarks v eenv = // decide whether to use a shadow local or not let useShadowLocal = cenv.opts.generateDebugSymbols && - not cenv.opts.localOptimizationsAreOn && + not cenv.opts.localOptimizationsEnabled && not v.IsCompilerGenerated && not v.IsMutable && // Don't use shadow locals for things like functions which are not compiled as static values/properties diff --git a/src/fsharp/IlxGen.fsi b/src/fsharp/IlxGen.fsi index b178fd4bc3b..7fe28f1616b 100644 --- a/src/fsharp/IlxGen.fsi +++ b/src/fsharp/IlxGen.fsi @@ -31,7 +31,7 @@ type internal IlxGenOptions = mainMethodInfo: Attribs option /// Indicates if local optimizations are active - localOptimizationsAreOn: bool + localOptimizationsEnabled: bool /// Indicates if we are generating debug symbols or not generateDebugSymbols: bool diff --git a/src/fsharp/OptimizeInputs.fs b/src/fsharp/OptimizeInputs.fs index 95a174e4aeb..f1bb56b1b8c 100644 --- a/src/fsharp/OptimizeInputs.fs +++ b/src/fsharp/OptimizeInputs.fs @@ -173,7 +173,7 @@ let GenerateIlxCode workAroundReflectionEmitBugs=tcConfig.isInteractive // REVIEW: is this still required? generateDebugSymbols= tcConfig.debuginfo fragName = fragName - localOptimizationsAreOn= tcConfig.optSettings.localOpt () + localOptimizationsEnabled= tcConfig.optSettings.LocalOptimizationsEnabled testFlagEmitFeeFeeAs100001 = tcConfig.testFlagEmitFeeFeeAs100001 mainMethodInfo= mainMethodInfo ilxBackend = ilxBackend diff --git a/src/fsharp/Optimizer.fs b/src/fsharp/Optimizer.fs index 878495e08d5..e4058646d99 100644 --- a/src/fsharp/Optimizer.fs +++ b/src/fsharp/Optimizer.fs @@ -349,59 +349,64 @@ type OptimizationSettings = } /// Determines if JIT optimizations are enabled - member x.jitOpt() = match x.jitOptUser with Some f -> f | None -> jitOptDefault + member x.JitOptimizationsEnabled = match x.jitOptUser with Some f -> f | None -> jitOptDefault /// Determines if intra-assembly optimization is enabled - member x.localOpt () = match x.localOptUser with Some f -> f | None -> localOptDefault + member x.LocalOptimizationsEnabled = match x.localOptUser with Some f -> f | None -> localOptDefault /// Determines if cross-assembly optimization is enabled member x.crossAssemblyOpt () = - x.localOpt () && + x.LocalOptimizationsEnabled && x.crossAssemblyOptimizationUser |> Option.defaultValue crossAssemblyOptimizationDefault /// Determines if we should keep optimization values member x.KeepOptimizationValues = x.crossAssemblyOpt () /// Determines if we should inline calls - member x.InlineLambdas = x.localOpt () + member x.InlineLambdas = x.LocalOptimizationsEnabled /// Determines if we should eliminate unused bindings with no effect - member x.EliminateUnusedBindings = x.localOpt () + member x.EliminateUnusedBindings = x.LocalOptimizationsEnabled /// Determines if we should arrange things so we debug points for pipelines x |> f1 |> f2 /// including locals "", "" and so on. /// On by default for debug code. member x.DebugPointsForPipeRight = - not (x.localOpt ()) && + not x.LocalOptimizationsEnabled && x.debugPointsForPipeRight |> Option.defaultValue debugPointsForPipeRightDefault + /// Determines if we should eliminate for-loops around an expr if it has no effect + /// + /// This optimization is off by default, given tiny overhead of including try/with. See https://github.com/Microsoft/visualfsharp/pull/376 + member x.EliminateForLoop = x.LocalOptimizationsEnabled + /// Determines if we should eliminate try/with or try/finally around an expr if it has no effect /// /// This optimization is off by default, given tiny overhead of including try/with. See https://github.com/Microsoft/visualfsharp/pull/376 member _.EliminateTryWithAndTryFinally = false /// Determines if we should eliminate first part of sequential expression if it has no effect - member x.EliminateSequential = x.localOpt () + member x.EliminateSequential = x.LocalOptimizationsEnabled /// Determines if we should determine branches in pattern matching based on known information, e.g. /// eliminate a "if true then .. else ... " - member x.EliminateSwitch = x.localOpt () + member x.EliminateSwitch = x.LocalOptimizationsEnabled /// Determines if we should eliminate gets on a record if the value is known to be a record with known info and the field is not mutable - member x.EliminateRecdFieldGet = x.localOpt () + member x.EliminateRecdFieldGet = x.LocalOptimizationsEnabled /// Determines if we should eliminate gets on a tuple if the value is known to be a tuple with known info - member x.EliminateTupleFieldGet = x.localOpt () + member x.EliminateTupleFieldGet = x.LocalOptimizationsEnabled /// Determines if we should eliminate gets on a union if the value is known to be that union case and the particular field has known info - member x.EliminateUnionCaseFieldGet () = x.localOpt () + member x.EliminateUnionCaseFieldGet () = x.LocalOptimizationsEnabled /// Determines if we should eliminate non-compiler generated immediate bindings - member x.EliminateImmediatelyConsumedLocals() = x.localOpt () + member x.EliminateImmediatelyConsumedLocals() = x.LocalOptimizationsEnabled /// Determines if we should expand "let x = (exp1, exp2, ...)" bindings as prior tmps /// Also if we should expand "let x = Some exp1" bindings as prior tmps - member x.ExpandStructuralValues() = x.localOpt () + member x.ExpandStructuralValues() = x.LocalOptimizationsEnabled type cenv = { g: TcGlobals @@ -2453,7 +2458,7 @@ and OptimizeFastIntegerForLoop cenv env (spStart, v, e1, dir, e2, e3, m) = let einfos = [e1info;e2info;e3info] let eff = OrEffects einfos (* neither bounds nor body has an effect, and loops always terminate, hence eliminate the loop *) - if not eff then + if cenv.settings.EliminateForLoop && not eff then mkUnit cenv.g m, { TotalSize=0; FunctionSize=0; HasEffect=false; MightMakeCriticalTailcall=false; Info=UnknownValue } else let exprR = mkFor cenv.g (spStart, v, e1R, dir, e2R, e3R, m) @@ -3333,7 +3338,7 @@ and OptimizeDebugPipeRights cenv env expr = xs0R inputVals pipesExprR - expr, pipesInfo + expr, { pipesInfo with HasEffect=true} and OptimizeFSharpDelegateInvoke cenv env (invokeRef, f0, f0ty, tyargs, args, m) = let g = cenv.g @@ -3529,7 +3534,7 @@ and OptimizeMatch cenv env (spMatch, exprm, dtree, targets, m, ty) = and OptimizeMatchPart2 cenv (spMatch, exprm, dtreeR, targetsR, dinfo, tinfos, m, ty) = let newExpr, newInfo = RebuildOptimizedMatch (spMatch, exprm, m, ty, dtreeR, targetsR, dinfo, tinfos) - let newExpr2 = if not (cenv.settings.localOpt()) then newExpr else CombineBoolLogic newExpr + let newExpr2 = if not cenv.settings.LocalOptimizationsEnabled then newExpr else CombineBoolLogic newExpr newExpr2, newInfo and CombineMatchInfos dinfo tinfo = @@ -3754,7 +3759,7 @@ and OptimizeModuleExpr cenv env x = let _renaming, hidden as rpi = ComputeRemappingFromImplementationToSignature cenv.g def mty let def = - if not (cenv.settings.localOpt()) then def else + if not cenv.settings.LocalOptimizationsEnabled then def else let fvs = freeInModuleOrNamespace CollectLocals def let dead = diff --git a/src/fsharp/Optimizer.fsi b/src/fsharp/Optimizer.fsi index f1a798afc0a..2f58475338a 100644 --- a/src/fsharp/Optimizer.fsi +++ b/src/fsharp/Optimizer.fsi @@ -43,9 +43,9 @@ type OptimizationSettings = } - member jitOpt: unit -> bool + member JitOptimizationsEnabled: bool - member localOpt: unit -> bool + member LocalOptimizationsEnabled: bool static member Defaults: OptimizationSettings diff --git a/src/fsharp/fsi/fsi.fs b/src/fsharp/fsi/fsi.fs index 13983a01d36..f798664cc3b 100644 --- a/src/fsharp/fsi/fsi.fs +++ b/src/fsharp/fsi/fsi.fs @@ -1114,7 +1114,7 @@ type internal FsiDynamicCompiler let valuePrinter = FsiValuePrinter(fsi, tcConfigB, tcGlobals, generateDebugInfo, resolveAssemblyRef, outWriter) - let assemblyBuilder,moduleBuilder = mkDynamicAssemblyAndModule (assemblyName, tcConfigB.optSettings.localOpt(), generateDebugInfo, fsiCollectible) + let assemblyBuilder,moduleBuilder = mkDynamicAssemblyAndModule (assemblyName, tcConfigB.optSettings.LocalOptimizationsEnabled, generateDebugInfo, fsiCollectible) let rangeStdin = rangeN stdinMockFilename 0 diff --git a/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs b/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs index 3f842a0c049..1975fc7a5a8 100644 --- a/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs +++ b/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs @@ -6,10 +6,10 @@ open FSharp.Test open NUnit.Framework [] -module ``BooleanLogic`` = +module BooleanLogic = [] - let ``BooleanOrs``() = + let BooleanOrs() = CompilerAssert.CompileLibraryAndVerifyILWithOptions [|"-g"; "--optimize+"|] """ module BooleanOrs @@ -54,3 +54,59 @@ let compute (x: int) = """ ]) +[] +// We had a regression in debug code regression where we were falsely marking pipelines +// as non-side-effecting, causing them to be eliminated in loops. +// +// After the fix +// 1. pipelines are correctly marked as having effect +// 2. we don't eliminate loops anyway +module DontEliminateForLoopsInDebugCode = + + CompilerAssert.CompileLibraryAndVerifyILWithOptions [|"-g"; "--optimize-"|] + """ +module DontEliminateForLoops + +let unsolved = [true] +let ApplyDefaults () = + + for priority = 0 to 10 do + unsolved |> List.iter (fun tp -> System.Console.WriteLine()) + + """ + (fun verifier -> verifier.VerifyIL [ + """ +.method public static int32 compute(int32 x) cil managed + { + + .maxstack 8 + IL_0000: nop + IL_0001: ldarg.0 + IL_0002: ldc.i4.1 + IL_0003: beq.s IL_0009 + + IL_0005: ldarg.0 + IL_0006: ldc.i4.2 + IL_0007: bne.un.s IL_000b + + IL_0009: ldc.i4.2 + IL_000a: ret + + IL_000b: nop + IL_000c: ldarg.0 + IL_000d: ldc.i4.3 + IL_000e: beq.s IL_0014 + + IL_0010: ldarg.0 + IL_0011: ldc.i4.4 + IL_0012: bne.un.s IL_0016 + + IL_0014: ldc.i4.3 + IL_0015: ret + + IL_0016: ldc.i4.4 + IL_0017: ret +} + """ + ]) + From 2c22bccb2e51b1e153f0fe530718c2171861efd4 Mon Sep 17 00:00:00 2001 From: Don Syme Date: Mon, 23 Aug 2021 23:09:47 +0100 Subject: [PATCH 2/2] update test --- .../CodeGen/EmittedIL/BooleanLogic.fs | 90 ++++++++++++------- 1 file changed, 60 insertions(+), 30 deletions(-) diff --git a/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs b/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs index 1975fc7a5a8..2a981b237f5 100644 --- a/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs +++ b/tests/fsharp/Compiler/CodeGen/EmittedIL/BooleanLogic.fs @@ -63,6 +63,9 @@ let compute (x: int) = // 2. we don't eliminate loops anyway module DontEliminateForLoopsInDebugCode = + [] + // See https://github.com/dotnet/fsharp/pull/12021 + let Regression12021() = CompilerAssert.CompileLibraryAndVerifyILWithOptions [|"-g"; "--optimize-"|] """ module DontEliminateForLoops @@ -76,37 +79,64 @@ let ApplyDefaults () = """ (fun verifier -> verifier.VerifyIL [ """ -.method public static int32 compute(int32 x) cil managed - { +.method public static void ApplyDefaults() cil managed +{ + + .maxstack 5 + .locals init (int32 V_0, + class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 V_1, + class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2 V_2, + class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 V_3, + class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 V_4, + class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 V_5, + bool V_6) + IL_0000: ldc.i4.0 + IL_0001: stloc.0 + IL_0002: br.s IL_004b - .maxstack 8 - IL_0000: nop - IL_0001: ldarg.0 - IL_0002: ldc.i4.1 - IL_0003: beq.s IL_0009 - - IL_0005: ldarg.0 - IL_0006: ldc.i4.2 - IL_0007: bne.un.s IL_000b - - IL_0009: ldc.i4.2 - IL_000a: ret - - IL_000b: nop - IL_000c: ldarg.0 - IL_000d: ldc.i4.3 - IL_000e: beq.s IL_0014 - - IL_0010: ldarg.0 - IL_0011: ldc.i4.4 - IL_0012: bne.un.s IL_0016 - - IL_0014: ldc.i4.3 - IL_0015: ret - - IL_0016: ldc.i4.4 - IL_0017: ret -} + IL_0004: call class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 DontEliminateForLoops::get_unsolved() + IL_0009: stloc.1 + IL_000a: ldsfld class DontEliminateForLoops/ApplyDefaults@8 DontEliminateForLoops/ApplyDefaults@8::@_instance + IL_000f: stloc.2 + IL_0010: ldloc.1 + IL_0011: stloc.3 + IL_0012: ldloc.3 + IL_0013: stloc.s V_4 + IL_0015: ldloc.s V_4 + IL_0017: call instance class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1::get_TailOrNull() + IL_001c: stloc.s V_5 + IL_001e: ldloc.s V_5 + IL_0020: ldnull + IL_0021: cgt.un + IL_0023: brfalse.s IL_0047 + + IL_0025: ldloc.s V_4 + IL_0027: call instance !0 class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1::get_HeadOrDefault() + IL_002c: stloc.s V_6 + IL_002e: ldloc.2 + IL_002f: ldloc.s V_6 + IL_0031: callvirt instance !1 class [FSharp.Core]Microsoft.FSharp.Core.FSharpFunc`2::Invoke(!0) + IL_0036: pop + IL_0037: ldloc.s V_5 + IL_0039: stloc.s V_4 + IL_003b: ldloc.s V_4 + IL_003d: call instance class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1 class [FSharp.Core]Microsoft.FSharp.Collections.FSharpList`1::get_TailOrNull() + IL_0042: stloc.s V_5 + IL_0044: nop + IL_0045: br.s IL_001e + + IL_0047: ldloc.0 + IL_0048: ldc.i4.1 + IL_0049: add + IL_004a: stloc.0 + IL_004b: ldloc.0 + IL_004c: ldc.i4.1 + IL_004d: ldc.i4.s 10 + IL_004f: add + IL_0050: blt.s IL_0004 + + IL_0052: ret +} """ ])