Adds optimized emit for int64 constants #3620
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request. |
forki
reviewed
Sep 21, 2017
src/fsharp/IlxGen.fs
Outdated
| @@ -2072,7 +2072,13 @@ and GenConstant cenv cgbuf eenv (c,m,ty) sequel = | |||
| | Const.SByte i -> CG.EmitInstr cgbuf (pop 0) (Push [ilTy]) (mkLdcInt32 (int32 i)) | |||
| | Const.Int16 i -> CG.EmitInstr cgbuf (pop 0) (Push [ilTy]) (mkLdcInt32 (int32 i)) | |||
| | Const.Int32 i -> CG.EmitInstr cgbuf (pop 0) (Push [ilTy]) (mkLdcInt32 i) | |||
| | Const.Int64 i -> CG.EmitInstr cgbuf (pop 0) (Push [ilTy]) (iLdcInt64 i) | |||
| | Const.Int64 i -> | |||
| if i >= int64 System.Int32.MinValue && i <= int64 System.Int32.MaxValue then | |||
There was a problem hiding this comment.
please add a comment here that links to this issue here
There was a problem hiding this comment.
As this is only a PR, should we make a dedicated issue?
There was a problem hiding this comment.
for me this is fine since you explained the situation
There was a problem hiding this comment.
Hm sorry for disturbing but I disagree, we should open an issue for this so people can find it if necessary in the issues.
There was a problem hiding this comment.
If the contributors reach consensus I am happy to create another summary for this in an issue ;)
|
I'm a bit surprised that this doesn't break any tests ;-) |
|
Lucky me, right :D
What kind of tests would you expect to fail? This might help me to write some meaning full tests :D
In the end this is nothing else but creating an int and casting it to an Int64. Don't know yet, how this could be testet properly yet.
…________________________________
From: Steffen Forkmann <notifications@github.com>
Sent: Thursday, September 21, 2017 11:48:02 AM
To: Microsoft/visualfsharp
Cc: Christian Steinert; Author
Subject: Re: [Microsoft/visualfsharp] [WIP] Adds optimized emit for int64 constants (#3620)
I'm a bit surprised that this doesn't break any tests ;-)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#3620 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AJaTxe5jkwqUsKSbxiOz4FaUdd8g856Bks5skjDRgaJpZM4Pe-9P>.
|
|
We have tests that check the generated IL. So we would want to copy on of
those. I will send you a link to one example later today
Am 21.09.2017 12:17 schrieb "Christian Steinert" <notifications@github.com>:
… Lucky me, right :D
What kind of tests would you expect to fail? This might help me to write
some meaning full tests :D
In the end this is nothing else but creating an int and casting it to an
Int64. Don't know yet, how this could be testet properly yet.
________________________________
From: Steffen Forkmann ***@***.***>
Sent: Thursday, September 21, 2017 11:48:02 AM
To: Microsoft/visualfsharp
Cc: Christian Steinert; Author
Subject: Re: [Microsoft/visualfsharp] [WIP] Adds optimized emit for int64
constants (#3620)
I'm a bit surprised that this doesn't break any tests ;-)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<https://github.com/
dotnet/fsharp#3620#issuecomment-331108058>, or mute the
thread<https://github.com/notifications/unsubscribe-auth/
AJaTxe5jkwqUsKSbxiOz4FaUdd8g856Bks5skjDRgaJpZM4Pe-9P>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3620 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNFieGjgNs80Ekimm36mgPWcuE2vnks5skjfQgaJpZM4Pe-9P>
.
|
|
Wow - this does look like some black magic, tbh. |
|
It would be so much fun if a compiler pr would not be a little challenging, right? |
|
Do we know what impact this has on the size and perf of jitted or ngened code? My intuition is none ... I imagine there is a minor reduction in size of the il for many 64 bit constants. |
|
Kevin maybe you find one of your C# colleagues who knows such stuff and
then please cc him or her here. That would probably be a good help.
Am 21.09.2017 20:31 schrieb "Kevin Ransom (msft)" <notifications@github.com
…:
Do we know what impact this has on the size and perf of jitted or ngened
code? My intuition is none ... I imagine there is a minor reduction in size
of the il for many 64 bit constants.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3620 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNNn4bBjvrvJ-a3B1ELvWy0HrtycAks5skqt4gaJpZM4Pe-9P>
.
|
|
As posted initially, the reduction is ~70% for values zero to eight and ~30% for values that fit in |
|
In general this looks like an optimization the Jit should do, since it is an instruction set specific optimization and pretty straightforward and easy. Roslyn does it, because the native C# compiler did it. There are undoubtedly other reasons too including il size. I am surprised that the Jit doesn't already do it. |
|
I just had a chat with a mate on the Jit team he said .... Apparently the very best thing one can do for the jit is to give it as small an IL stream as you can get away with. |
|
hey so this works well ... but on FSharp.Compiler.dll we only seem to gain about 2k of IL size which for a 14 MB dll, is kind of low. Does anyone have any 64 bit heavy F# code, I would like to understand how impactfull this will actually be. Thanks Kevin |
KevinRansom
approved these changes
Sep 22, 2017
|
Oh wow. This came unexpected :) I thought about what kind of test I could add for this. |
|
For tests, I currently have something like this in mind: https://gist.github.com/ChrSteinert/62ffabe5c9a39a9be15e5613a1b3e099 Can I please get some guidance on whether this is a good thing to do and where to put it in the test folders? |
|
I actually think the assertion needs to be the emitted IL
Am 23.09.2017 19:04 schrieb "Christian Steinert" <notifications@github.com>:
… For tests, I currently have something like this in mind:
https://gist.github.com/ChrSteinert/62ffabe5c9a39a9be15e5613a1b3e099
Can I please get some guidance on whether this is a good thing to do and
where to put it in the test folders?
Thanks.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3620 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNEsRAUTUIgdYdxo-Sl1bYyErd2oKks5slTodgaJpZM4Pe-9P>
.
|
|
Please let me know if this is more what you had in mind and if I should put that into this PR as well: |
|
I'm oK with this change simply to align with C# |
|
@ChrSteinert you may submit tests in a separate PR. The generated IL is fine, and has benefit. Albeit super minor. The risk of this change is also very low. |
ChrSteinert
changed the title
KevinRansom
added a commit
that referenced
this pull request
Oct 11, 2017
* Generate source for .resx files on build. (#3607) * add build task to generate *.fs from *.resx files * generate source for embedded resources in tests * generate source for embedded resources in FSharp.Editor * generate source for embedded resources in FSharp.LanguageService * generate source for embedded resources in FSharp.ProjectSystem.FSharp * generate source for embedded resources in FSharp.VS.FSI * don't generate non-string resources when <=netstandard1.6 * update baseline error message for tests The error output should be the exception message, not the exception type. * perform up-to-date check before generating *.fs from *.resx * remove non-idiomatic fold for an array comprehension * correct newline replacement * output more friendly error message * throw if boolean value isn't explicitly `true` or `false` * only generate object resource code on non `netstandard1.*` and `netcoreapp1.*` platforms * ensure FSharp.Core specifies a target framework for resource generaton * rename attributes to be non-ambiguous and properly include them * fix order of file items in FSharp.Core * Fix build.cmd for certain always-shown errors (like "unable to find registry key"), improve finding of VS2017 tools, fix DEVGUIDE.md (#3635) * Fix build.com displaying the following error on each run "ERROR: The system was unable to find the specified registry key or value." * Remove warning about reg.exe errors introduced in #3614 (in commit b548bd7, but unrelated to that fix), it is no longer necessary. * Fix #3638, VS2017 Preview installation was not found when VS2017RTM is not installed. Remove comment introduced in #3614 (through commit 966bd7f) * Fixing JaroWinkler tests with InvariantCulture and fixing async tests by removing Debugger.Break() (#3627) * Fixing JaroWinkler tests to use InvariantCulture for number-to-string * Fixing the crashing of test runners because of a Debugger.Break() in a test * update to System.Collections.Immutable 1.3.1 (#3641) * update to System.Collections.Immutable 1.3.1 * fixes * fix assembly reference * [WIP] Adds optimized emit for int64 constants (#3620) * Adds optimized emit for int64 constants. * Adds comment linking to the changing PR. Thanks for this PR. Kevin * fix assembly reference (#3646) * Remove a few more build warnings (#3647) * fix assembly reference * remove more build warnings * fix build * move BuildFromSource projects to their own directory * Adds tests for emitted IL for new Int64 constants. (#3650) * Enable FS as prefix and ignore invalid values for warnings (#3631) * enable fs as prefix and ignore invalid values for warnings + tests * Allow #pragma to validate warnings * do it right * use ordinal compare * In both places * Add fs prefix to warnaserror * Fixup tests * Fix stack overflow on assembly resolution (#3658) * Fix stack overflow on tp assembly resolution * Feedback * Add impl files to file check results (#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (#3657) * bump FCS version (#3676) * bump version * Update RELEASE_NOTES.md * Parsing improvements: no reactor, add parsing options, error severity options (#3601) * Parse without reactor, add parsing options, error severity options * Revert parsing APIs (fallback to the new ones), fix VFT projects * Cache parse results after type check * Add impl files to file check results (#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (#3657) * bump FCS version (#3676) * bump version * Update RELEASE_NOTES.md * updates to make tests pass * restore old behaviour of CheckFileInProjectAllowingStaleCachedResults (builder had been created by ParseFileInProject) * restore use of CheckFileInProjectAllowingStaleCachedResults * deprecate test relying on whacky behaviour of deprecated GetCheckResultsBeforeFileInProjectEvenIfStale * Use ParseFile and FSharpParsingOptions instead of ParseFileInProject * prepare FCS release with this feature * whitespace cleanup (#3682) * whitespace and docs (#3684) * Preserve XML docs for in-memory project references (#3683) * fix xmldocs for in-memory project references * add test * fix tests * whitespace and comments (#3686) * fix assembly reference * whitespace and comments * whitespace and comments * whitespace and comments * cherry pick two PRs from FCS (#3687) * fix assembly reference * remove line endings from all *.nuspec files * ProjectCracker returns *.fsi files in FSharpProjectOptions.SourceFiles array (in addition to *.fs files, in right order) * ProjectCracker raises exception if ProjectCrackerTool returns non null ProjectCrackerOptions.Error (new field) * fix build on linux * fix a test * slashes * revert slashes * Update FSharp.Compiler.Service.ProjectCracker.nuspec * try to fix travis * try to fix travis * list dependencies * no obsolete pdb in nuget * list dependencies * cherry pick of fsharp/fsharp change * bump FCS version number (#3688) * Update FSharp.Compiler.Service.MSBuild.v12.nuspec * fix FCS nuget on windows * fix-resource (#3690) * Bump FSharp.Compiler.Tools to 4.1.27 and align mono build files (#3693) * ri change from fsharp * fix test * bump FSC tools to 4.1.27 * remove fsharp.core from Mono GAC * align mono directory * fix typo * install back versions with Mono * fix typo * update FCS doc generation (#3694) * update DEVGUIDE to add addiitional steps before running build (#3725) * Split templates out into a seperate vsix (#3720) * merge error * Merge issues
nosami
pushed a commit
to xamarin/visualfsharp
that referenced
this pull request
Jan 26, 2022
* Generate source for .resx files on build. (dotnet#3607) * add build task to generate *.fs from *.resx files * generate source for embedded resources in tests * generate source for embedded resources in FSharp.Editor * generate source for embedded resources in FSharp.LanguageService * generate source for embedded resources in FSharp.ProjectSystem.FSharp * generate source for embedded resources in FSharp.VS.FSI * don't generate non-string resources when <=netstandard1.6 * update baseline error message for tests The error output should be the exception message, not the exception type. * perform up-to-date check before generating *.fs from *.resx * remove non-idiomatic fold for an array comprehension * correct newline replacement * output more friendly error message * throw if boolean value isn't explicitly `true` or `false` * only generate object resource code on non `netstandard1.*` and `netcoreapp1.*` platforms * ensure FSharp.Core specifies a target framework for resource generaton * rename attributes to be non-ambiguous and properly include them * fix order of file items in FSharp.Core * Fix build.cmd for certain always-shown errors (like "unable to find registry key"), improve finding of VS2017 tools, fix DEVGUIDE.md (dotnet#3635) * Fix build.com displaying the following error on each run "ERROR: The system was unable to find the specified registry key or value." * Remove warning about reg.exe errors introduced in dotnet#3614 (in commit b548bd7, but unrelated to that fix), it is no longer necessary. * Fix dotnet#3638, VS2017 Preview installation was not found when VS2017RTM is not installed. Remove comment introduced in dotnet#3614 (through commit 966bd7f) * Fixing JaroWinkler tests with InvariantCulture and fixing async tests by removing Debugger.Break() (dotnet#3627) * Fixing JaroWinkler tests to use InvariantCulture for number-to-string * Fixing the crashing of test runners because of a Debugger.Break() in a test * update to System.Collections.Immutable 1.3.1 (dotnet#3641) * update to System.Collections.Immutable 1.3.1 * fixes * fix assembly reference * [WIP] Adds optimized emit for int64 constants (dotnet#3620) * Adds optimized emit for int64 constants. * Adds comment linking to the changing PR. Thanks for this PR. Kevin * fix assembly reference (dotnet#3646) * Remove a few more build warnings (dotnet#3647) * fix assembly reference * remove more build warnings * fix build * move BuildFromSource projects to their own directory * Adds tests for emitted IL for new Int64 constants. (dotnet#3650) * Enable FS as prefix and ignore invalid values for warnings (dotnet#3631) * enable fs as prefix and ignore invalid values for warnings + tests * Allow #pragma to validate warnings * do it right * use ordinal compare * In both places * Add fs prefix to warnaserror * Fixup tests * Fix stack overflow on assembly resolution (dotnet#3658) * Fix stack overflow on tp assembly resolution * Feedback * Add impl files to file check results (dotnet#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (dotnet#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (dotnet#3657) * bump FCS version (dotnet#3676) * bump version * Update RELEASE_NOTES.md * Parsing improvements: no reactor, add parsing options, error severity options (dotnet#3601) * Parse without reactor, add parsing options, error severity options * Revert parsing APIs (fallback to the new ones), fix VFT projects * Cache parse results after type check * Add impl files to file check results (dotnet#3659) * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project * add ImplementationFiles to FSharpCheckFileResults * make FSharpImplementationFileContents ctor internal * throw if ImplementationFiles is called having keepAssemblyContents flag set to false * add a test * spelling and cosmetics * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd (dotnet#3672) * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * This adds backup, restore, coloration and many more checks to the update-vsintegration.cmd * Remove ambiguous an irrelevant instruction, improved help and instructions * Fix a scenario where the return code wasn't nonzero for error conditions, fixes not creating backup dir when not backing up * add LanguageServiceProfiling project to internals visible to list of FSharp.Compiler.Private project (dotnet#3657) * bump FCS version (dotnet#3676) * bump version * Update RELEASE_NOTES.md * updates to make tests pass * restore old behaviour of CheckFileInProjectAllowingStaleCachedResults (builder had been created by ParseFileInProject) * restore use of CheckFileInProjectAllowingStaleCachedResults * deprecate test relying on whacky behaviour of deprecated GetCheckResultsBeforeFileInProjectEvenIfStale * Use ParseFile and FSharpParsingOptions instead of ParseFileInProject * prepare FCS release with this feature * whitespace cleanup (dotnet#3682) * whitespace and docs (dotnet#3684) * Preserve XML docs for in-memory project references (dotnet#3683) * fix xmldocs for in-memory project references * add test * fix tests * whitespace and comments (dotnet#3686) * fix assembly reference * whitespace and comments * whitespace and comments * whitespace and comments * cherry pick two PRs from FCS (dotnet#3687) * fix assembly reference * remove line endings from all *.nuspec files * ProjectCracker returns *.fsi files in FSharpProjectOptions.SourceFiles array (in addition to *.fs files, in right order) * ProjectCracker raises exception if ProjectCrackerTool returns non null ProjectCrackerOptions.Error (new field) * fix build on linux * fix a test * slashes * revert slashes * Update FSharp.Compiler.Service.ProjectCracker.nuspec * try to fix travis * try to fix travis * list dependencies * no obsolete pdb in nuget * list dependencies * cherry pick of fsharp/fsharp change * bump FCS version number (dotnet#3688) * Update FSharp.Compiler.Service.MSBuild.v12.nuspec * fix FCS nuget on windows * fix-resource (dotnet#3690) * Bump FSharp.Compiler.Tools to 4.1.27 and align mono build files (dotnet#3693) * ri change from fsharp * fix test * bump FSC tools to 4.1.27 * remove fsharp.core from Mono GAC * align mono directory * fix typo * install back versions with Mono * fix typo * update FCS doc generation (dotnet#3694) * update DEVGUIDE to add addiitional steps before running build (dotnet#3725) * Split templates out into a seperate vsix (dotnet#3720) * merge error * Merge issues
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the emitted IL for Int64 constants and aligns it with what Roslyn is doing for C#.
Why?
The new emitted IL is smaller in code size.
Currently if you create a Int64 value, the compiler emits an
ldc.i8 <value>, that is an 9 byte instruction (one byte encoding the instruction (0x21) and 8 bytes the actual value). This is quite a lot. Roslyn takes a shortcut there (see here: http://source.roslyn.io/#Microsoft.CodeAnalysis/CodeGen/ILBuilderEmit.cs,642). It creates an Int32 and converts it to an Int64. As Int32 even has special instructions for values 0 to 8, this produces methods of less code size.Done so far
Still missing
I remember seeing a blog post somewhere ranting about F#'s emitted IL for Int64, but unfortunately cannot find it anymore. Alas, I picked the same logic that Roslyn applies where. Can't be this wrong then, can it? ;)
Let me know of anything that is missing and how to proceed on this.