Optimizer: eliminate unused Unchecked.defaultof<'T> bindings#19758
Optimizer: eliminate unused Unchecked.defaultof<'T> bindings#19758Copilot wants to merge 23 commits into
Unchecked.defaultof<'T> bindings#19758Conversation
EI_ilzero (Unchecked.defaultof) as effect-free to enable dead binding elimination
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
…for concrete types Unchecked.defaultof<'T> (compiled as EI_ilzero) was unconditionally treated as having an effect, preventing the optimizer from eliminating unused bindings like `let _ = Unchecked.defaultof<decimal>`. Fix: In ExprHasEffect and OptimizeExprOpFallback, EI_ilzero is now considered effect-free when all type args are concrete (not type parameters). When type variables are present (e.g. SRTP ^T), it is conservatively treated as having an effect to prevent orphaned type variables during IL generation (FS0073). Fixes #17775 Co-authored-by: Copilot <[email protected]>
8b99fdc to
7ad68b7
Compare
isTyparTy only detects direct type parameters (e.g. ^T) but not type variables nested inside constructed types (e.g. SomeType<^T>). Replace with freeInTypes CollectTyparsNoCaching which recursively checks for any free type parameters in the type structure. Also add release notes entry. Co-authored-by: Copilot <[email protected]>
Broaden the type-variable check to cover ALL effect-free Expr.Op expressions, not just EI_ilzero. When any operation would be effect-free but its F# type args contain free type parameters, conservatively treat it as having an effect. This prevents dead binding/sequential elimination from orphaning type variables that are only referenced through the eliminated expression. Also add EI_ilzero to the instruction-level no-effect list (its safety is ensured by the tyargs check above it in the pipeline). Also add release notes entry. Co-authored-by: Copilot <[email protected]>
17e17e9 to
ccefa82
Compare
T-Gro
left a comment
There was a problem hiding this comment.
Review: Optimizer – treat EI_ilzero as effect-free
Overall: Solid fix. The root-cause analysis is correct, the code change is minimal, and both positive and negative test cases are included. Approve direction; a few observations below.
1. Breadth of the ExprHasEffect / OptimizeExprOpFallback type-var guard
The freeInTypes check is now applied to all Expr.Op nodes that are otherwise effect-free, not only TOp.ILAsm [EI_ilzero _].
This means dead-binding elimination is also blocked for unused tuples, union cases, struct records, etc. when their tyargs reference a free typar – even the function's own type parameter (which is safe to reference via the IL generic context).
Example that could be affected:
sharp let f<'T> () = let _ = Unchecked.defaultof<'T> // safe to eliminate, 'T is the function's own typar 42
In practice this is unlikely to matter (who writes unused defaultof with their own typar?), but it would be worth adding a comment explaining the trade-off: "we accept false positives for non-SRTP typars to keep the fix simple."
If you later want a tighter check, you could limit the guard to TOp.ILAsm containing EI_ilzero, since that's the only instruction that can appear with zero value-args in an expression whose sole purpose is providing an SRTP witness/dummy.
2. Duplicate logic – consider a helper
The same freeInTypes CollectTyparsNoCaching tyargs check appears verbatim at lines 1642 and 2663. A small helper like:
sharp let tyargsHaveFreeTypars tyargs = not (List.isEmpty tyargs) && not (Zset.isEmpty (freeInTypes CollectTyparsNoCaching tyargs).FreeTypars)
would reduce duplication and make intent clearer at both call sites.
3. Performance consideration
freeInTypes traverses the type list and allocates a FreeTyvars. It's now called on every Expr.Op with non-empty tyargs whose op+args are effect-free. In large optimized builds this could add up. Have you spot-checked compiler self-build time or BenchmarkDotNet traces to confirm it's negligible? The "NoCaching" variant means no memoization across calls.
4. Tests
Good coverage:
- Concrete type → eliminated ✓
- SRTP type var (including nested
Wrapper<^T>) → preserved ✓
Optional addition: a test with a non-SRTP generic function demonstrating the conservative "keep" behavior (to document expected behavior and guard against future over-optimization):
sharp let f<'T> () = let _ = Unchecked.defaultof<'T> 42
5. Minor: release-notes wording
"for concrete types"
The optimization applies whenever tyargs have no free typars, so Unchecked.defaultof<list<int>> is also eliminated – "concrete" is correct but might be clearer as "fully-resolved types" or "types without free type parameters."
Overall: the fix is correct and safe. The conservatism is acceptable. Nice work.
…test The previous fix conservatively marked every Expr.Op whose tyargs contained free typars as having an effect. That over-broad check regresses dead-binding elimination for many unrelated generic constructions (e.g. let _ = Some x). Push the check down to where it actually matters: TOp.ILAsm with EI_ilzero. Only EI_ilzero embeds tyargs into IL and is the only newly-effect-free instruction introduced by this PR, so it is the only case that can orphan a free typar and trip FS0073. OpHasEffect now takes tyargs; ExprHasEffect / OptimizeExprOpFallback go back to a single line. Replaced bloated comments and List.isEmpty checks with pattern matching on []. Tests: split the bundled SRTP test into typar and nested-typar cases, and added Generic_Some_unused_binding_still_eliminated to lock in that the fix does not penalize other effect-free generic operations. Co-authored-by: Copilot <[email protected]>
Adversarial review of PR #19758 surfaced three blockers: 1. The tyargsContainFreeTypars protection guarding EI_ilzero against orphaning free typars was never exercised. With it neutered to always return false, 447 EmittedIL + 728 EmittedIL.RealInternalSignature tests pass, including the actual issue #18128 SRTP witness pattern (FSharpPlus-style 'nil<^b>' inside an inline _call helper). No FS0073 reproducer exists. Remove the protection. The fix is now one line: add EI_ilzero to IlAssemblyCodeInstrHasEffect's no-effect match. OpHasEffect signature is restored, ILAsmHasEffect helper is removed. 2. The branch was based on a stale main and silently reverted 14+ unrelated release-notes entries. Restore the file from origin/main and re-apply only our entry on top, with corrected wording that does not understate the fix (the original 'for concrete types' qualifier is no longer accurate and never was useful to users). 3. The PR referenced issue #17775, which is an automated VS insertion PR list, not an EI_ilzero bug. The real issue is #18128. Update test annotations and the release-notes entry accordingly. Tests reorganised to three distinct cases that each pin observable behaviour: - Issue_18128_Unchecked_defaultof_concrete_eliminated: verifyILNotPresent ["initobj"] - the fix's stated purpose - Issue_18128_SRTP_witness_pattern_compiles_and_optimizes: the actual FSharpPlus-style pattern from the issue body - Generic_Some_unused_binding_still_eliminated: non-regression that proves EI_ilzero is targeted, not a broader effect-analysis change Co-authored-by: Copilot <[email protected]>
EI_ilzero (Unchecked.defaultof) as effect-free to enable dead binding eliminationUnchecked.defaultof<'T> bindings
…rgon Per round-2 expert review: - Strengthen concrete-elimination test to also forbid any Decimal local slot (catches partial regressions where 1 of 3 bindings survives). - Pin the SRTP doWork body exactly to its eliminated form (ldarg + ldc.r8 + mul + ret); the previous shouldSucceed-only assertion did not catch optimizer regressions on the witness path. - Add a cctor-soundness test documenting that defaultof<refType> emits ldnull (not newobj), so eliminating the binding cannot suppress an observable cctor side-effect that previously occurred. f's body is pinned to its 2-instruction post-optimisation form. - Drop the Generic_Some test - it passed on main without this PR, so it was not a regression catcher for the EI_ilzero change. - Reword the release-notes entry to drop the internal opcode name and describe the user-visible effect. Co-authored-by: Copilot <[email protected]>
Per round-3 expert review: - Drop the stray blank line at Optimizer.fs:2645; final functional diff vs main is now exactly one line. - Rename the cctor-soundness test from 'does_not_run_cctor' to 'leaves_no_reference_to_T_in_caller'. The previous name implied a runtime guarantee, but the assertion is purely IL-shape: it verifies that f's body contains no reference to WithCctor (defaultof of a reference type lowers to ldnull, so the cctor was never reachable through f to begin with). Co-authored-by: Copilot <[email protected]>
Two CI failures addressed: 1. FS0073 in FSharpPlus regression tests: The previous commit stripped the tyargsContainFreeTypars protection, claiming no reproducer exists. FSharpPlus NET10 builds prove otherwise - SRTP witness patterns like nil<^b> where the type variable remains unsolved at optimization time need the binding kept. Restored via ILAsmWithIlzeroHasEffect: when EI_ilzero instructions are present and tyargs contain free type parameters, treat the ILAsm as effectful. 2. IL format mismatch in SRTP test: Different CI configurations (NoRealsig, CompressedMetadata) output ldc.r8 2. instead of ldc.r8 2. Changed the test from exact IL body matching to verifyILNotPresent which tests the actual optimization behavior (absence of initobj/ldnull) without sensitivity to float literal formatting. Co-authored-by: Copilot <[email protected]>
…-defaultof-optimization
|
🔍 Tooling Safety Check — Affects-Compiler-Output
|
The previous if/else structure would skip the IlAssemblyCodeHasEffect check entirely when EI_ilzero was present. Rewrite to always check the standard instruction-level effects, then additionally flag the free-typar ilzero guard. This is more robust if EI_ilzero ever appears alongside other instructions. Co-authored-by: Copilot <[email protected]>
The previous check used freeInTypes which follows type parameter solutions via stripTyparEqns. This could mark EI_ilzero as effect-free when the tyarg was a solved SRTP type variable (e.g. ^a = SomeType), because freeInTypes would see the solution as concrete. However, in complex SRTP scenarios (like FSharpPlus operators), intermediate anonymous type variables referenced through solved SRTPs can trip FS0073 in IlxGen when the binding is eliminated. The fix checks the tree-level type node directly: only TType_app, TType_tuple, etc. pass the safety check. Any TType_var (even solved ones) is conservatively treated as effectful, preventing elimination. This still enables the optimization for the common case: let _ = Unchecked.defaultof<decimal> // eliminated (TType_app) let _ = Unchecked.defaultof<MyRecord> // eliminated (TType_app) Co-authored-by: Copilot <[email protected]>
…-defaultof-optimization
Eliminate Unchecked.defaultof bindings only when tyargs have no free typars, fixing FS0073 in SRTP-heavy code (FSharpPlus). Co-authored-by: Copilot <[email protected]>
The optimizer cannot determine per-op whether eliminating an EI_ilzero (Unchecked.defaultof) binding is safe: even ground-tyarg bindings can pin sibling SRTP typars resolved later, causing FS0073 in IlxGen on FSharpPlus monad CEs. Revert to main behavior. Co-authored-by: Copilot <[email protected]>
|
@T-Gro most recent commit reverted all changes |
…-defaultof-optimization
This reverts commit 1bcc1c2.
- Restore CodeGenRegressions_Observations.fs to main (it is a pre-existing file; the SRTP defaultof tests now live in the focused UncheckedDefaultofOptimization.fs). - UncheckedDefaultofOptimization.fs: 4 focused tests (concrete elimination, SRTP witness elimination, cctor soundness, FS0073 guard for unsolved typars). - Optimizer.fs: keep IlAssemblyCodeInstrHasEffect conservative for EI_ilzero; the only effect-free path for ilzero is the fully-ground-tyargs guard in ILAsmWithIlzeroHasEffect. Co-authored-by: Copilot <[email protected]>
…inline bodies Fixes the FSharpPlus NET10 regression (FS0073 'Undefined or unsolved type variable') introduced by eliminating unused Unchecked.defaultof<'T> bindings. Root cause: the elimination was applied while optimizing inline value bodies, whose optimized form is pickled as cross-assembly optimization info and re-inlined at each use site. Dropping an ilzero SRTP witness/dummy binding there corrupts that info; a consumer's IlxGen then finds a closure free type variable that is no longer pinned and reports FS0073. Elimination is safe at the fully-instantiated use site itself, so no codegen quality is lost. Fix: gate ilzero elimination on cenv.optimizing (false inside inline bodies). Only eliminate when optimizing for emission AND the erased tyargs are fully ground. Threads the flag through the effect-analysis helpers; the public ExprHasEffect entry point keeps emission-time semantics. Updates the SRTP-witness test to assert elimination at the use site (doWork), since an inline function's compiler-generated dynamic-invocation stub legitimately still contains ldnull. Co-authored-by: Copilot <[email protected]>
…-defaultof-optimization
|
@abonie : Its difficult to find a balance between doing this optimization and not breaking a special FSharpPlus usage which intentionally uses a typed defaultof (with specified generic argument) to influence SRTP resolution. I need more time here, I would really love to get this in. |
Unchecked.defaultof<T>unused bindings were not eliminated even with optimizations enabled, because the optimizer's effect analysis incorrectly classified the underlyingEI_ilzeroIL instruction as having side effects.Root cause
Unchecked.defaultof<T>inlines to(# "ilzero !0" type ('T) : 'T #), which the optimizer represents asTOp.ILAsm([EI_ilzero ty], ...). TheIlAssemblyCodeInstrHasEffectfunction inOptimizer.fsdid not recognizeEI_ilzeroas effect-free, so it fell through to the conservative_ -> truedefault, preventing dead binding elimination.Change
src/Compiler/Optimize/Optimizer.fs— addEI_ilzero _to the no-effect branch ofIlAssemblyCodeInstrHasEffect. The instruction only produces a zero/default value (mapping toldnull,ldc.i4.0, orinitobjdepending on type), with no observable side effects.This is particularly relevant for SRTP-based patterns (common in libraries like FSharpPlus) where
Unchecked.defaultof<'T>is used as a witness/dummy argument to drive overload resolution and ends up as unused bindings at call sites.