Skip to content

Reduce IL generation in GetHashCode for Discriminated Unions #2096

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.

Sign up for GitHub

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

Merged
KevinRansom merged 4 commits into from
Dec 29, 2016
Merged

Reduce IL generation in GetHashCode for Discriminated Unions #2096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc6938b
return the union's tag when case is nullary
liboz Dec 24, 2016
2d2a841
fix IL tests
liboz Dec 25, 2016
8d36e8d
ensure that gethashcode is not inlined
liboz Dec 27, 2016
f8474f6
only for compilergenerated cases now
liboz Dec 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 29 additions & 20 deletions src/fsharp/AugmentWithHashCompare.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -626,27 +626,36 @@ let mkUnionHashWithComparer g tcref (tycon:Tycon) compe =
let mkCase i ucase1 =
let c1ref = tcref.MakeNestedUnionCaseRef ucase1
let m = c1ref.Range
let mkHash thise j (rfield:RecdField) =
let fty = rfield.FormalType
let e = mkUnionCaseFieldGetProvenViaExprAddr(thise, c1ref, tinst, j, m)
mkCallGenericHashWithComparerOuter g m fty compe e

let test =
if tycon.IsStructOrEnumTycon then
mkCompGenSequential m
(mkValSet m (mkLocalValRef accv) (mkInt g m i))
(mkCombineHashGenerators g m (List.mapi (mkHash thise) ucase1.RecdFields) (mkLocalValRef accv) acce)
else
let ucv,ucve = mkCompGenLocal m "unionCase" (mkProvenUnionCaseTy c1ref tinst)
mkCompGenLet m ucv
(mkUnionCaseProof (thise,c1ref,tinst,m))
(mkCompGenSequential m
if ucase1.IsNullary then None
else
let mkHash thise j (rfield:RecdField) =
let fty = rfield.FormalType
let e = mkUnionCaseFieldGetProvenViaExprAddr(thise, c1ref, tinst, j, m)
mkCallGenericHashWithComparerOuter g m fty compe e

let test =
if tycon.IsStructOrEnumTycon then
mkCompGenSequential m
(mkValSet m (mkLocalValRef accv) (mkInt g m i))
(mkCombineHashGenerators g m (List.mapi (mkHash ucve) ucase1.RecdFields) (mkLocalValRef accv) acce))

mkCase(Test.UnionCase(c1ref,tinst),mbuilder.AddResultTarget(test,SuppressSequencePointAtTarget))

let dtree = TDSwitch(thise, List.mapi mkCase ucases, None,m)
(mkCombineHashGenerators g m (List.mapi (mkHash thise) ucase1.RecdFields) (mkLocalValRef accv) acce)
else
let ucv,ucve = mkCompGenLocal m "unionCase" (mkProvenUnionCaseTy c1ref tinst)
mkCompGenLet m ucv
(mkUnionCaseProof (thise,c1ref,tinst,m))
(mkCompGenSequential m
(mkValSet m (mkLocalValRef accv) (mkInt g m i))
(mkCombineHashGenerators g m (List.mapi (mkHash ucve) ucase1.RecdFields) (mkLocalValRef accv) acce))
Some(mkCase(Test.UnionCase(c1ref,tinst),mbuilder.AddResultTarget(test,SuppressSequencePointAtTarget)))

let nullary,nonNullary = ucases
Copy link
Contributor

@dsyme dsyme Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally please use formatting

let x,y = 
    abc def

rather than

let x,y = abc 
           def

Copy link
Contributor Author

@liboz liboz Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll try to remember to do so in the future. One quick clarification though: I assume it's alright if there are enough pipes in a row to put pipes on their own lines?

Also, do you want me to fix it in this PR?

|> List.mapi mkCase
|> List.partition (fun i -> i.IsNone)
let cases = nonNullary |> List.map (function (Some c) -> c | None -> failwith "mkUnionHash")
let dflt = if isNil nullary then None
else
let tag = mkUnionCaseTagGetViaExprAddr (thise,tcref,tinst,m)
Some(mbuilder.AddResultTarget(tag,SuppressSequencePointAtTarget))
let dtree = TDSwitch(thise, cases, dflt,m)
let stmt = mbuilder.Close(dtree,m,g.int_ty)
let expr = mkCompGenLet m accv (mkZero g m) stmt
let expr = if tycon.IsStructOrEnumTycon then expr else mkBindNullHash g m thise expr
Expand Down
20 changes: 15 additions & 5 deletions src/fsharp/Optimizer.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2500,7 +2500,7 @@ and TryDevirtualizeApplication cenv env (f,tyargs,args,m) =
| _ -> None

/// Attempt to inline an application of a known value at callsites
and TryInlineApplication cenv env (_f0',finfo) (tyargs: TType list,args: Expr list,m) =
and TryInlineApplication cenv env finfo (tyargs: TType list,args: Expr list,m) =
// Considering inlining app
match finfo.Info with
| StripLambdaValue (lambdaId,arities,size,f2,f2ty) when
Expand All @@ -2522,6 +2522,8 @@ and TryInlineApplication cenv env (_f0',finfo) (tyargs: TType list,args: Expr li
match args.[0] with
| Expr.Val(vref,_,_) when vref.BaseOrThisInfo = BaseVal -> true
| _ -> false

if isBaseCall then None else

// Since Lazy`1 moved from FSharp.Core to mscorlib on .NET 4.0, inlining Lazy values from 2.0 will
// confuse the optimizer if the assembly is referenced on 4.0, since there will be no value to tie back
Expand All @@ -2539,15 +2541,23 @@ and TryInlineApplication cenv env (_f0',finfo) (tyargs: TType list,args: Expr li
| _ -> false
| _ -> false
| _ -> false


if isValFromLazyExtensions then None else

let isSecureMethod =
match finfo.Info with
| ValValue(vref,_) ->
vref.Attribs |> List.exists (fun a -> (IsSecurityAttribute cenv.g cenv.amap cenv.casApplied a m) || (IsSecurityCriticalAttribute cenv.g a))
| _ -> false

if isBaseCall || isSecureMethod || isValFromLazyExtensions then None
else
if isSecureMethod then None else

let isGetHashCode =
match finfo.Info with
| ValValue(vref,_) -> vref.DisplayName = "GetHashCode" && vref.IsCompilerGenerated
| _ -> false

if isGetHashCode then None else

// Inlining lambda
(* ---------- printf "Inlining lambda near %a = %s\n" outputRange m (showL (exprL f2)) (* JAMES: *) ----------*)
Expand Down Expand Up @@ -2577,7 +2587,7 @@ and OptimizeApplication cenv env (f0,f0ty,tyargs,args,m) =
res
| None ->
let newf0,finfo = OptimizeExpr cenv env f0
match TryInlineApplication cenv env (newf0,finfo) (tyargs,args,m) with
match TryInlineApplication cenv env finfo (tyargs,args,m) with
| Some res ->
// inlined
res
Expand Down
47 changes: 11 additions & 36 deletions tests/fsharpqa/Source/CodeGen/EmittedIL/CCtorDUWithMember/CCtorDUWithMember01.il.bsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,61 +406,36 @@
instance int32 GetHashCode(class [mscorlib]System.Collections.IEqualityComparer comp) cil managed
{
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
// Code size 40 (0x28)
// Code size 15 (0xf)
.maxstack 3
.locals init (int32 V_0,
class CCtorDUWithMember01a/C V_1,
class CCtorDUWithMember01a/C V_2)
.locals init (int32 V_0)
IL_0000: nop
IL_0001: ldarg.0
IL_0002: ldnull
IL_0003: cgt.un
IL_0005: brfalse.s IL_0009

IL_0007: br.s IL_000b

IL_0009: br.s IL_0026

IL_0009: br.s IL_0014
IL_000b: ldc.i4.0
IL_000c: stloc.0
IL_000d: ldarg.0
IL_000e: call instance int32 CCtorDUWithMember01a/C::get_Tag()
IL_0013: ldc.i4.0
IL_0014: bne.un.s IL_0018

IL_0016: br.s IL_001a

IL_0018: br.s IL_0020

IL_001a: ldarg.0
IL_001b: stloc.1
IL_001c: ldc.i4.0
IL_001d: stloc.0
IL_001e: ldloc.0
IL_001f: ret

IL_0020: ldarg.0
IL_0021: stloc.2
IL_0022: ldc.i4.1
IL_0023: stloc.0
IL_0024: ldloc.0
IL_0025: ret

IL_0026: ldc.i4.0
IL_0027: ret
IL_000e: ldfld int32 CCtorDUWithMember01a/C::_tag
IL_0013: ret
IL_0014: ldc.i4.0
IL_0015: ret
} // end of method C::GetHashCode

.method public hidebysig virtual final
instance int32 GetHashCode() cil managed
{
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
// Code size 13 (0xd)
.maxstack 4
.line 3,3 : 6,7
// Code size 21 (0x15)
.maxstack 8
.line 3,3 : 6,7 ''
IL_0000: nop
IL_0001: ldarg.0
IL_0002: call class [mscorlib]System.Collections.IEqualityComparer [FSharp.Core]Microsoft.FSharp.Core.LanguagePrimitives::get_GenericEqualityComparer()
IL_0007: call instance int32 CCtorDUWithMember01a/C::GetHashCode(class [mscorlib]System.Collections.IEqualityComparer)
IL_0007: callvirt instance int32 CCtorDUWithMember01a/C::GetHashCode(class [mscorlib]System.Collections.IEqualityComparer)
IL_000c: ret
} // end of method C::GetHashCode

Expand Down
43 changes: 9 additions & 34 deletions tests/fsharpqa/Source/CodeGen/EmittedIL/CCtorDUWithMember/cctorduwithmember01.il.netfx4.bsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,57 +409,32 @@
instance int32 GetHashCode(class [mscorlib]System.Collections.IEqualityComparer comp) cil managed
{
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
// Code size 40 (0x28)
// Code size 15 (0xf)
.maxstack 3
.locals init (int32 V_0,
class CCtorDUWithMember01a/C V_1,
class CCtorDUWithMember01a/C V_2)
.locals init (int32 V_0)
IL_0000: nop
IL_0001: ldarg.0
IL_0002: ldnull
IL_0003: cgt.un
IL_0005: brfalse.s IL_0009

IL_0007: br.s IL_000b

IL_0009: br.s IL_0026

IL_0009: br.s IL_0014
IL_000b: ldc.i4.0
IL_000c: stloc.0
IL_000d: ldarg.0
IL_000e: call instance int32 CCtorDUWithMember01a/C::get_Tag()
IL_0013: ldc.i4.0
IL_0014: bne.un.s IL_0018

IL_0016: br.s IL_001a

IL_0018: br.s IL_0020

IL_001a: ldarg.0
IL_001b: stloc.1
IL_001c: ldc.i4.0
IL_001d: stloc.0
IL_001e: ldloc.0
IL_001f: ret

IL_0020: ldarg.0
IL_0021: stloc.2
IL_0022: ldc.i4.1
IL_0023: stloc.0
IL_0024: ldloc.0
IL_0025: ret

IL_0026: ldc.i4.0
IL_0027: ret
IL_000e: ldfld int32 CCtorDUWithMember01a/C::_tag
IL_0013: ret
IL_0014: ldc.i4.0
IL_0015: ret
} // end of method C::GetHashCode

.method public hidebysig virtual final
instance int32 GetHashCode() cil managed
{
.custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
// Code size 13 (0xd)
// Code size 21 (0x15)
.maxstack 8
.line 3,3 : 6,7
.line 3,3 : 6,7 ''
IL_0000: nop
IL_0001: ldarg.0
IL_0002: call class [mscorlib]System.Collections.IEqualityComparer [FSharp.Core]Microsoft.FSharp.Core.LanguagePrimitives::get_GenericEqualityComparer()
Expand Down
78 changes: 36 additions & 42 deletions tests/fsharpqa/Source/CodeGen/EmittedIL/Misc/EqualsOnUnions01.il.bsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,62 +622,56 @@
// Code size 72 (0x48)
.maxstack 7
.locals init (int32 V_0,
class EqualsOnUnions01/U/_A V_1,
class EqualsOnUnions01/U/B V_2,
class [mscorlib]System.Collections.IEqualityComparer V_3)
class EqualsOnUnions01/U/B V_1,
class [mscorlib]System.Collections.IEqualityComparer V_2,
class EqualsOnUnions01/U V_3)
IL_0000: nop
IL_0001: ldarg.0
IL_0002: ldnull
IL_0003: cgt.un
IL_0005: brfalse.s IL_0009

IL_0007: br.s IL_000b

IL_0009: br.s IL_0046

IL_0009: br.s IL_004a
IL_000b: ldc.i4.0
IL_000c: stloc.0
IL_000d: ldarg.0
IL_000e: isinst EqualsOnUnions01/U/_A
IL_000e: isinst EqualsOnUnions01/U/B
IL_0013: brfalse.s IL_0017

IL_0015: br.s IL_0019

IL_0017: br.s IL_0024

IL_0017: br.s IL_003b
IL_0019: ldarg.0
IL_001a: castclass EqualsOnUnions01/U/_A
IL_001a: castclass EqualsOnUnions01/U/B
IL_001f: stloc.1
IL_0020: ldc.i4.0
IL_0020: ldc.i4.1
IL_0021: stloc.0
IL_0022: ldloc.0
IL_0023: ret

IL_0024: ldarg.0
IL_0025: castclass EqualsOnUnions01/U/B
IL_002a: stloc.2
IL_002b: ldc.i4.1
IL_002c: stloc.0
IL_002d: ldc.i4 0x9e3779b9
IL_0032: ldarg.1
IL_0033: stloc.3
IL_0034: ldloc.2
IL_0035: ldfld int32 EqualsOnUnions01/U/B::item
IL_003a: ldloc.0
IL_003b: ldc.i4.6
IL_003c: shl
IL_003d: ldloc.0
IL_003e: ldc.i4.2
IL_003f: shr
IL_0040: add
IL_0041: add
IL_0042: add
IL_0043: stloc.0
IL_0044: ldloc.0
IL_0045: ret

IL_0046: ldc.i4.0
IL_0047: ret
IL_0022: ldc.i4 0x9e3779b9
IL_0027: ldarg.1
IL_0028: stloc.2
IL_0029: ldloc.1
IL_002a: ldfld int32 EqualsOnUnions01/U/B::item
IL_002f: ldloc.0
IL_0030: ldc.i4.6
IL_0031: shl
IL_0032: ldloc.0
IL_0033: ldc.i4.2
IL_0034: shr
IL_0035: add
IL_0036: add
IL_0037: add
IL_0038: stloc.0
IL_0039: ldloc.0
IL_003a: ret
IL_003b: ldarg.0
IL_003c: stloc.3
IL_003d: ldloc.3
IL_003e: isinst EqualsOnUnions01/U/B
IL_0043: brfalse.s IL_0048
IL_0045: ldc.i4.1
IL_0046: br.s IL_0049
IL_0048: ldc.i4.0
IL_0049: ret
IL_004a: ldc.i4.0
IL_004b: ret
} // end of method U::GetHashCode

.method public hidebysig virtual final
Expand Down
Loading