Optimize in FCS by forki · Pull Request #3784 · dotnet/fsharp

forki · 2017-10-20T12:36:14Z

In Fable we use the FCS to get parsed files:

https://github.com/fable-compiler/Fable/blob/03bc8e46f0fc94c70d2ae89d533784d10cf84d58/src/dotnet/dotnet-fable/StateUtil.fs#L107

/cc @ncave @alfonsogarciacaro

Corresponding FABLE PR: fable-compiler/Fable#1197

forki · 2017-10-20T12:45:11Z

what's that outFile?

forki · 2017-10-20T15:18:36Z

/cc @kjnilsson probably also interesting for fez

kjnilsson · 2017-10-20T17:08:07Z

Looks fantastic.

dsyme · 2017-10-25T00:18:05Z

@forki Impressive. I took a very quick glance and it look totally feasible. I thought this would need much more work.

forki · 2017-10-25T01:54:35Z

@dsyme what we need is to flag the optimizer for creating ilasm statements. These are hard to translate back in fable

ncave · 2017-11-26T17:08:59Z

@forki IMO we just need to add the missing ILAsm conversions in Exprs.fs.
I added a few here as a test, let me know if you want me to submit them to your PR.
Here is a test REPL so you can try them live: https://ncave.github.io/fable-repl-test

forki · 2017-11-26T17:11:17Z

Yes that would be awesome Am 26.11.2017 18:09 schrieb "ncave" <notifications@github.com>:

…

@forki <https://github.com/forki> IMO we just need to add the missing ILAsm conversions in Exprs.fs. I added a few here <ncave/FSharp.Compiler.Service@8518748> as a test, let me know if you want me to submit them to your PR. Here is a test REPL so you can try them live: https://ncave.github.io/fable-repl-test — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3784 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNC_0YZ5Z8KJ4S9sDLj9nnULD1Korks5s6ZsrgaJpZM4QAm4R> .

ncave · 2017-11-26T19:21:16Z

@forki See PR#37.

alfonsogarciacaro · 2017-11-29T00:23:05Z

So is this already working? Will it be merged? Or can we make a custom build for Fable? I used the Fable.FCS package in the past for that, we could reuse it now 😄

ncave · 2017-11-29T00:26:27Z

@alfonsogarciacaro All good questions idk the answer to. But you can definitely go ahead with a custom build if you want to try it. It's unfortunate that there are no FCS build artefacts for netstandard.

ncave · 2017-11-29T00:58:46Z

@alfonsogarciacaro Just FYI if you are custom building the FCS for netstandard, you may have to fix the embedded resource name in the .fsproj a little bit like that:

    <EmbeddedResource Include="$(FSharpSourcesRoot)/fsharp/FSStrings.resx">
      <Link>FSStrings.resx</Link>
      <LogicalName>fsstrings.resources</LogicalName>
    </EmbeddedResource>

ncave · 2017-11-29T01:08:41Z

@alfonsogarciacaro There are still a few more ILAsm that need to be properly translated before it can pass all Fable tests, (e.g. I_box, I_call).

alfonsogarciacaro · 2017-11-29T06:17:59Z

@ncave Thanks a lot for the info! Hmm, I don't remember doing anything in particular last time I did a custom build, but it was FCS repo (which I'm much more familiarized with) not this one. Would it be possible to create a branch with these changes on your FCS fork?

forki · 2017-11-29T06:38:05Z

Dude, do you have any idea how much time I spent to make the FCS build a nuget from here? ;-) I just rebased and will try to grab the package from appveyor and update the fable pr. Then you can test it out. Ok? Am 29.11.2017 07:18 schrieb "Alfonso Garcia-Caro" <notifications@github.com

…

: @ncave <https://github.com/ncave> Thanks a lot for the info! Hmm, I don't remember doing anything in particular last time I did a custom build, but it was FCS repo (which I'm much more familiarized with) not this one. Would it be possible to create a branch with these changes on your FCS fork? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3784 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNETtQz7xkDcvEes87NbOELLq2nNvks5s7PcXgaJpZM4QAm4R> .

alfonsogarciacaro · 2017-11-29T06:54:31Z

Sounds like a plan! Awesome, thank you @forki 👏 🎉

ncave · 2017-11-29T11:38:20Z

@alfonsogarciacaro

Would it be possible to create a branch with these changes on your FCS fork?

Not a problem, but let me add the missing ILAsm translations first.

dsyme

This looks a very promising FCS feature. I don't see any outright errors - but we need to add testing.

Please find all the places where test assembly contents and also apply optimizations
Please check if we have a test that gets the assembly contents for FSharp.Core - I did do that manually but I don't think it's automated - ideally we would add this test and also get the optimized TAST for FSHarp.Core - that is an excellent stress test

dsyme · 2017-11-29T13:15:48Z

+
+    member info.AssemblyContents = FSharpAssemblyContents(info.TypedImplementionFiles)
+
+    member info.OptimizedAssemblyContents =  


Please change to GetOptimizedAssemblyContents() since it involves potentially very long computation (e.g. we don't want it triggering in the debugger when looking at one of these objects)

dsyme · 2017-11-29T13:16:36Z

+
+    member info.OptimizedAssemblyContents =  
+        let tcGlobals, thisCcu, tcImports, mimpls = info.TypedImplementionFiles
+        let outfile = null


What is outfile used for and can we pass in a correct (or at least non-null) value here?

dsyme

Request adding tests etc. per comment above

review fixes

more targeted replacements

ncave · 2018-02-02T13:25:48Z

@dsyme Thanks for the review, here are the changes since:

fixed checked ops
fixed checked conv
fixed typeof
fixed string hash
fixed dual conv
added more tests

…to optimize

Optimize

dsyme

One last set of additions please - can you also expand/multiply out your tests to include instantiations of the basic operators at the full matrix of primitive types? byte, sbyte, ..., UInt64, double, single, decimal, ... We may as well pin the whole lot down now and get everything under test?

added more tests

added even more tests

dsyme

I spotted a few cases where the tree being returned was inaccurate

This is extremely close to being done - I looked through everything else and it looked good.

dsyme · 2018-02-13T23:09:31Z

+        "let testSByteToSByteOperator(e1) = Operators.ToSByte<Microsoft.FSharp.Core.sbyte> (e1) @ (42,45--42,53)";
+        "let testSByteToInt16Operator(e1) = Operators.ToInt16<Microsoft.FSharp.Core.sbyte> (e1) @ (43,45--43,53)";
+        "let testSByteToUInt16Operator(e1) = Operators.ToUInt16<Microsoft.FSharp.Core.sbyte> (e1) @ (44,45--44,54)";
+        "let testSByteToIntOperator(e1) = e1 @ (45,45--45,51)";


I'm a little concerned about this - it looks like the code being returned wouldn't pass type checking. This will be because it's a no-op conversion

There are quite a lot of cases of this. See comment further above in ConvExprPrim for the root cause

dsyme · 2018-02-13T23:12:49Z

-
-            | TOp.ILAsm([ AI_ceq ], _), _, [arg1;arg2]  -> 
+            | TOp.ILAsm([ ], _), _, [arg] -> 
+                ConvExprPrim cenv env arg


This case is used for no-op conversions like sbyte --> int32.

I think in this case you strictly speaking need to insert an appropriately-typed conversion operator in the F# expression based on the type of arg and the type of expr

BTW ConvExprPrim is getting kind of huge and maybe should be chopped into ConvExprPrim and ConvOpExprPrim

dsyme · 2018-02-13T23:15:30Z

+        "let testByteToUInt64Checked(e1) = Checked.ToUInt64<Microsoft.FSharp.Core.byte> (e1) @ (37,43--37,60)";
+        "let testByteToIntPtrChecked(e1) = Checked.ToIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (38,43--38,63)";
+        "let testByteToUIntPtrChecked(e1) = Checked.ToUIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (39,43--39,64)";
+        "let testByteToByteOperator(e1) = O


let testInt64ToCharOperator(e1) = Operators.ToUInt16<Microsoft.FSharp.Core.int64> (e1)

This looks wrong. I think the IL instruction sequence is ambiguous and needs to be disamiguated by the type of expr in ConvExprPrim

dsyme · 2018-02-13T23:17:56Z

+        "let testByteToUInt64Checked(e1) = Checked.ToUInt64<Microsoft.FSharp.Core.byte> (e1) @ (37,43--37,60)";
+        "let testByteToIntPtrChecked(e1) = Checked.ToIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (38,43--38,63)";
+        "let testByteToUIntPtrChecked(e1) = Checked.ToUIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (39,43--39,64)";
+        "let testByteToByteOperator(e1) = O


Here's another char one where we've ended up with ToUInt16 (see comment below)

dsyme · 2018-02-13T23:18:14Z

+        "let testByteToUInt64Checked(e1) = Checked.ToUInt64<Microsoft.FSharp.Core.byte> (e1) @ (37,43--37,60)";
+        "let testByteToIntPtrChecked(e1) = Checked.ToIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (38,43--38,63)";
+        "let testByteToUIntPtrChecked(e1) = Checked.ToUIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (39,43--39,64)";
+        "let testByteToByteOperator(e1) = O


another char --> UInt16 mistake (See comment below)

dsyme · 2018-02-13T23:19:03Z

+        "let testByteToUInt64Checked(e1) = Checked.ToUInt64<Microsoft.FSharp.Core.byte> (e1) @ (37,43--37,60)";
+        "let testByteToIntPtrChecked(e1) = Checked.ToIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (38,43--38,63)";
+        "let testByteToUIntPtrChecked(e1) = Checked.ToUIntPtr<Microsoft.FSharp.Core.byte> (e1) @ (39,43--39,64)";
+        "let testByteToByteOperator(e1) = O


another char/UInt16 mistake (See comment below)

review fixes

dsyme · 2018-02-14T14:52:48Z

The changes are good, thank you! Just a few warnings left I think:

  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(276,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(277,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(278,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(279,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(280,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(281,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(282,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(283,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(284,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(285,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(286,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(287,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]
  D:\j\w\debug_default---ce4d11dd\src\fsharp\symbols\Exprs.fs(288,15): error FS1182: The value 'x' is unused [D:\j\w\debug_default---ce4d11dd\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]

dsyme

A few warnings to remove, but approved - great work on testing and thank you for going the extra distance to make this a robust feature of FCS

ncave · 2018-02-14T16:43:10Z

@dsyme Thank you for reviewing so thoroughly, the end result is always so much better for it.
And thank you @forki for coming up with this original idea and blazing the trail.

forki · 2018-02-14T18:53:05Z

I was just sitting here and merging the PRs that went to my fork. Lol Am 14.02.2018 17:43 schrieb "ncave" <notifications@github.com>:

…

@dsyme <https://github.com/dsyme> Thank you for reviewing so thoroughly, the end result is always so much better for it. And thank you @forki <https://github.com/forki> for coming up with this original idea and blazing the trail. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3784 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNNDSqMogn_v2gDPUePU3tNQfrmzoks5tUw0fgaJpZM4QAm4R> .

fixed warnings

AviAvni · 2018-02-14T19:01:07Z

@forki you can offer this as a service to some repos 😉

forki · 2018-02-17T09:10:26Z

Awesome! This makes me happy

realvictorprm · 2018-02-17T10:27:56Z

This is great! Thank you all for your work! 😃 Please take a cookie as thanks 🍪 !

dsyme · 2018-02-22T12:39:49Z

+        "member M2(__) (unitVar1) = __.compiledAsInstanceMethod(()) @ (56,21--56,47)";
+        "member SM1(unitVar0) = Operators.op_Addition<Microsoft.FSharp.Core.int,Microsoft.FSharp.Core.int,Microsoft.FSharp.Core.int> (compiledAsStaticField,ClassWithImplicitConstructor.compiledAsGenericStaticMethod<Microsoft.FSharp.Core.int> (compiledAsStaticField)) @ (57,26--57,101)";
+        "member SM2(unitVar0) = ClassWithImplicitConstructor.compiledAsStaticMethod (()) @ (58,26--58,50)";
+#if NO_PROJECTCRACKER // proxy for COMPILER


@ncave Do you recall why this was needed? I don't think it's either NO_PROJECTCRACKER nor COMPILER. I'm hitting cases where this test is now failing and I'm not sure what I've done....

@dsyme This just means the output of FSharp.LanguageService is slightly different than FCS, and accounts for that.

These two projects reference FCS:
fcs/FSharp.Compiler.Service.Tests.netcore/FSharp.Compiler.Service.Tests.netcore.fsproj
fcs/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj

but vsintegration/tests/unittests/VisualFSharp.UnitTests.fsproj references FSharp.LanguageService, so a symbol defined in VisualFSharp.UnitTests.fsproj is used to branch the output.

I don't know what is the error, perhaps the output is now the same? (if so, then there is no need to branch it anymore).

FSharp.LanguageService

I think you mean FSharp.Compiler.Private being used from VisualFSharp.UnitTests.fsproj

In #4368 I'm now always seeing the output in the NO_PROJECTCRACKER branch even from the two FCS test projects. I don't particularly understand why though or what's changed. Don't worry about it for now - let's see if CI passes

* Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests (#40) * Add option to toggle unused declarations analyzer (#4074) * Add option to toggle unused declarations analyzer * Better name and handle registering code fixes. This will ensure that if someone uses warnon:1182, we won't suggest fixes if they've turned off the feature. * Revert #1650 (and #3366) (#4173) * Fix error logging in brace matching code (#4140) * Remove error logger pushing code * Update service.fs * Fix #4200: Vsix: fix empty "New file" window for web projects (#4202) * LOC CHECKIN | visualfsharp - master | 20180112 (#4194) * Fixed FCS netcore tests (#4180) * Remove ambiguous resolution error FS0332 (#4170) * Add IsInteractive to parsing options for script load closures (#4169) * Add IsInteractive to FSharpParsingOptions * Add test * Set serializable bit for all serializable types (#4211) * Minor fix (#4195) on string 58. * Symbols API: add Index to active pattern case, Name to pattern group (#4222) * Symbols API: add Index to active pattern case, Name to pattern group * Symbols API: add active pattern case use tests * don't rebuild (#4230) * Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests * test conditions update * test update * test condition update * test update * review update * added checked operators * fixed dual conversions * review fixes * more targeted replacements * adapt to latest * added more tests * added more tests * review fixes * fixed warnings

forki force-pushed the optimizeinfcs branch from bc8cbc5 to 3a62b07 Compare October 20, 2017 13:08

forki mentioned this pull request Oct 20, 2017

WIP: Try to optimize in FCS fable-compiler/Fable#1197

Closed

forki changed the title ~~Optimize in FCS~~ [WIP] Optimize in FCS Oct 20, 2017

forki force-pushed the optimizeinfcs branch from 3a62b07 to 7b54ce3 Compare October 20, 2017 14:14

forki force-pushed the optimizeinfcs branch from da8e810 to 52545d6 Compare October 20, 2017 17:15

forki force-pushed the optimizeinfcs branch from 8b7d693 to 336172f Compare October 25, 2017 08:21

forki closed this Oct 25, 2017

forki reopened this Oct 25, 2017

forki closed this Oct 26, 2017

forki reopened this Oct 26, 2017

forki force-pushed the optimizeinfcs branch from 2dd628e to 9feee86 Compare November 29, 2017 06:17

forki force-pushed the optimizeinfcs branch from 9feee86 to e5e1067 Compare November 29, 2017 06:19

dsyme reviewed Nov 29, 2017

View reviewed changes

dsyme requested changes Nov 29, 2017

View reviewed changes

ncave and others added 4 commits February 1, 2018 19:28

review fixes

de84cfd

Merge pull request #49 from ncave/optimize

c965f06

review fixes

more targeted replacements

1e373c7

Merge pull request #50 from ncave/optimize

adc20e2

more targeted replacements

ncave and others added 3 commits February 2, 2018 05:49

Merge branch 'master' of https://github.com/Microsoft/visualfsharp in…

03b4603

…to optimize

adapt to latest

e5f1c6f

Merge pull request #51 from ncave/optimize

c8743f4

Optimize

dsyme requested changes Feb 9, 2018

View reviewed changes

ncave and others added 4 commits February 9, 2018 10:02

added more tests

c7da1b4

Merge pull request #52 from ncave/optimize

6d14c03

added more tests

added more tests

a8dd212

Merge pull request #53 from ncave/optimize

e1f3e16

added even more tests

dsyme requested changes Feb 13, 2018

View reviewed changes

ncave and others added 2 commits February 14, 2018 04:03

review fixes

ddf74bd

Merge pull request #54 from ncave/optimize

d2906ad

review fixes

dsyme approved these changes Feb 14, 2018

View reviewed changes

fixed warnings

e15be74

Merge pull request #55 from ncave/optimize

da05e6b

fixed warnings

KevinRansom merged commit 2180de4 into dotnet:master Feb 17, 2018

dsyme reviewed Feb 22, 2018

View reviewed changes

alfonsogarciacaro mentioned this pull request Feb 22, 2018

Implement replacements for optimization switch fable-compiler/Fable#1266

Closed

35 tasks


		member info.AssemblyContents = FSharpAssemblyContents(info.TypedImplementionFiles)

		member info.OptimizedAssemblyContents =

Uh oh!

Conversation

forki commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forki commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forki commented Oct 20, 2017

Uh oh!

kjnilsson commented Oct 20, 2017

Uh oh!

dsyme commented Oct 25, 2017

Uh oh!

forki commented Oct 25, 2017

Uh oh!

ncave commented Nov 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

forki commented Nov 26, 2017 via email

Uh oh!

ncave commented Nov 26, 2017

Uh oh!

alfonsogarciacaro commented Nov 29, 2017

Uh oh!

ncave commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncave commented Nov 29, 2017

Uh oh!

ncave commented Nov 29, 2017

Uh oh!

alfonsogarciacaro commented Nov 29, 2017

Uh oh!

forki commented Nov 29, 2017 via email

Uh oh!

alfonsogarciacaro commented Nov 29, 2017

Uh oh!

ncave commented Nov 29, 2017

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

ncave commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsyme commented Feb 14, 2018

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

ncave commented Feb 14, 2018

Uh oh!

forki commented Oct 20, 2017 •

edited

Loading

forki commented Oct 20, 2017 •

edited

Loading

ncave commented Nov 26, 2017 •

edited

Loading

ncave commented Nov 29, 2017 •

edited

Loading

ncave commented Feb 2, 2018 •

edited

Loading

ncave Feb 22, 2018 •

edited

Loading