Skip to content
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

[WIP] Look for string.GetPinnableReference when pinning a string first #9252

Closed
wants to merge 2 commits into from
Closed

[WIP] Look for string.GetPinnableReference when pinning a string first #9252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ade88c
Look for string.GetPinnableReference when pinning a string first
cartermp May 20, 2020
3fc03c2
Merge branch 'master' into string-pinnable
KevinRansom May 22, 2020
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
21 changes: 18 additions & 3 deletions src/fsharp/TypeChecker.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3099,9 +3099,16 @@ let BuildDisposableCleanup cenv env m (v: Val) =
let BuildOffsetToStringData cenv env m =
let ad = env.eAccessRights
let offsetToStringDataMethod =
match TryFindIntrinsicOrExtensionMethInfo ResultCollectionSettings.AllResults cenv env m ad "get_OffsetToStringData" cenv.g.system_RuntimeHelpers_ty with
// Prefer string.GetPinnableReference introduced in .NET Core 3
cartermp marked this conversation as resolved.
Show resolved Hide resolved
match TryFindIntrinsicOrExtensionMethInfo ResultCollectionSettings.AllResults cenv env m ad "GetPinnableReference" cenv.g.string_ty with
| [x] -> x
| _ -> error(Error(FSComp.SR.tcCouldNotFindOffsetToStringData(), m))
| _ ->
// Otherwise, fall back to 'RuntimeHelpers.GetOffsetToStringData' (likely to happen if targeting an older platform)
match TryFindIntrinsicOrExtensionMethInfo ResultCollectionSettings.AllResults cenv env m ad "get_OffsetToStringData" cenv.g.system_RuntimeHelpers_ty with
| [x] -> x
| _ ->
// Otherwise, kaboom!
error(Error(FSComp.SR.tcCouldNotFindOffsetToStringData(), m))

let offsetExpr, _ = BuildPossiblyConditionalMethodCall cenv env NeverMutates m false offsetToStringDataMethod NormalValUse [] [] []
offsetExpr
Expand Down Expand Up @@ -11006,10 +11013,18 @@ and TcAndBuildFixedExpr cenv env (overallPatTy, fixedExpr, overallExprTy, mBindi
| ty when isStringTy cenv.g ty ->
let charPtrTy = mkNativePtrTy cenv.g cenv.g.char_ty
UnifyTypes cenv env mBinding charPtrTy overallPatTy

// When targeting .NET Core or higher:
//
// let ptr: nativeptr<char> =
// let pinned s = str
// (nativeptr)s + GetPinnableReference()
Copy link
Member

@GrabYourPitchforks GrabYourPitchforks May 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just to clarify, the compiler shouldn't perform any arithmetic here. The result of GetPinnableReference is a readonly char&, and that char& should be pinned. The string instance itself should not be pinned.

If you take a look at the "expected IL" example in #9251, the result of GetPinnableReference is immediately assigned to the pinned char& local. There's no add instruction in the expected IL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm. @dsyme wrote this originally, I assume he had a good reason for doing this, but yes it shouldn't need to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Clarification to my comments: the GetOffsetToStringData pattern works by having the caller pin the string instance, then compute addrof(string) + GetOffsetToStringData, then expose the resulting sum as a char*. So the addition is expected there.

The GetPinnableReference pattern works by providing the char& directly, which the caller then pins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I'll take another look at this

//
// Otherwise,
//
// let ptr: nativeptr<char> =
// let pinned s = str
// (nativeptr)s + get_OffsettoStringData()
// (nativeptr)s + get_OffsetToStringData()

mkCompGenLetIn mBinding "pinnedString" cenv.g.string_ty fixedExpr (fun (v, ve) ->
v.SetIsFixed()
Expand Down
37 changes: 37 additions & 0 deletions tests/fsharp/Compiler/CodeGen/EmittedIL/FixedExpressionCodeGen.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
namespace FSharp.Compiler.UnitTests.CodeGen.EmittedIL

open FSharp.Compiler.UnitTests
open NUnit.Framework

[<TestFixture>]
module FixedExpressionCodeGen =

#if NETCOREAPP
[<Test>]
let ``Pinning a string emits string.GetPinnableReference`` () =
let code = """
module FixedOnAString
#nowarn "9"
let foo(s : string) =
use ptr = fixed s
ptr
"""
let il = """
char& modreq([System.Runtime]System.Runtime.InteropServices.InAttribute) [System.Runtime]System.String::GetPinnableReference()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check to make sure the local char is pinned as well from locals init.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, thinking about it more this is a bad test. I'll revisit this one

"""
CompilerAssert.CompileLibraryAndVerifyIL code (fun verifier -> verifier.VerifyIL([ il ]))
#else
[<Test>]
let ``Pinning a string emits RuntimeHelpers.GetOffsetToStringData`` () =
let code = """
module FixedOnAString
#nowarn "9"
let foo(s : string) =
use ptr = fixed s
ptr
"""
let il = """
int32 [runtime]System.Runtime.CompilerServices.RuntimeHelpers::get_OffsetToStringData()
"""
CompilerAssert.CompileLibraryAndVerifyIL code (fun verifier -> verifier.VerifyIL([ il ]))
#endif
1 change: 1 addition & 0 deletions tests/fsharp/FSharpSuite.Tests.fsproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
<Compile Include="Compiler\ILChecker.fs" />
<Compile Include="Compiler\Utilities.fs" />
<Compile Include="Compiler\CompilerAssert.fs" />
<Compile Include="Compiler\CodeGen\EmittedIL\FixedExpressionCodeGen.fs" />
<Compile Include="Compiler\CodeGen\EmittedIL\StaticMember.fs" />
<Compile Include="Compiler\CodeGen\EmittedIL\StaticLinkTests.fs" />
<Compile Include="Compiler\CodeGen\EmittedIL\LiteralValue.fs" />
Expand Down