ok @dsyme was right. as always.

The optimizer is exposing new TAST elements that Fable did never seen before ;-)

but in other words: POC works

looks like it's always ILAsm

so the question is:
a) try to make fable understand it
b) can we disable ILAsm optimization?

@ncave @alfonsogarciacaro now I need help in the translation.
Can we work together on that?

@forki Is the BasicPatterns.ILAsm the only new element?

@ncave at least it's the one that we see failing in the tests

looks like ILAsm is the big new player. and then we have test failing with "Error: unexpected for-loop form"

So far I've only seen ILAsm to check an array length in pattern matching, interpreting it required a lot of debugging and wishful thinking. Probably Fable wouldn't have been possible if there were many more cases of ILAsm in the AST, as parsing those expressions requires much more work and a knowledge of IL that I don't have. Please also note that the embedded IL code is not typed as they're just string literals.

Personally I don't think handling an AST with lots of ILAsm is an option, though I've been proven wrong other times ;)

that would leave us with option a) and we need to figure out how to tell Optimizer to no ILAsm all the things

@forki Perhaps add to this if not already there?

@forki No, I just meant a setting can be added there (if not already there) to make it switchable, since from initial look I don't see a setting that already does that.

@ncave so you are saying adding another option to FCS optimization that disables ilasm replacement is the way to go?

@forki Well, if has to be added somewhere, the OptimizerSettings is as good a place as any.

I have to say, adding the optimization phase to FCS/Fable is a fabulous idea, it has a lot of potential to be a big differentiator in the market and I hope we can make it work. I'm really excited about it, especially since the other options for code/perf optimizations are still not quite usable.

Sometimes the best ideas are right there in the open, but somebody has to point them out. Bravo! 👏

@ncave I disabled FastIntergerLoopOptimization in FCS (as experiment) this should reduce number of failed tests dotnet/fsharp@07a30df

mhm now I'm lost. I'm not sure where all the ILAsm stuff is coming from. /cc @dsyme?

@forki Idk, perhaps everything else in TastOps.fs that returns mkAsmExpr, besides the ones used in 'for' loops.

@forki Perhaps turn OptimizeAllForExpressions to OptimizeIntRangesOnly in here (with a switch).

doesn't seem to heal it.

Here is a "optimize-able" REPL so you can try it live: https://ncave.github.io/fable-repl-test

Awesome!

EPIC WIN!!!!

I've been checking the REPL you set up @ncave and comparing it with Fable one. Really useful, thank you! In general the optimized version is emitting better code (when running the Ray Tracer the optimized version seems to get better times), sometimes it disables Fable optimization to flatten pipe chains creating extra closures (see capture, optimized code is on the right) but I guess this can be fixed.

I think it makes lot of sense we release a version that offers this as an experimental option with a compiler flag :)

BTW, did you also compiled FCS/Fable itself with the optimized version? Interestingly in your REPL FCS times seem to be faster but Fable times are slower.

And I forgot to say the optimization also resolves the trait calls (inline functions with statically resolved generic parameters), which would have saved a lot of headaches 😉

@ncave I updated this PR with the latest FCS optimizer. lot's of errors

ILAsm

I assume these come from inlining operators like +, i.e. all the different ILAsm forms will stem from uses of (# "... " #) in FSharp.Core? Or are you compiling against a different Fable.Core?

@alfonsogarciacaro

BTW, did you also compiled FCS/Fable itself with the optimized version?

No, not yet, we need all the missing ILAsm translations added first, but it's close.

@alfonsogarciacaro Update:

Unfortunately still not as close as I hoped for.
Here is a list of the outstanding errors introduced by the optimization when compiling the tests:

• error FABLE: Cannot find replacement for:

Microsoft.FSharp.Collections.FSharpList::CompareTo
Microsoft.FSharp.Collections.FSharpList::Equals
Microsoft.FSharp.Collections.FSharpList::GetHashCode
Microsoft.FSharp.Core.LanguagePrimitives.ErrorStrings::InputArrayEmptyString
Microsoft.FSharp.Core.LanguagePrimitives.ErrorStrings::InputMustBeNonNegativeString
Microsoft.FSharp.Core.LanguagePrimitives.ErrorStrings::InputSequenceEmptyString
Microsoft.FSharp.Core.LanguagePrimitives.HashCompare::GenericComparisonIntrinsic
Microsoft.FSharp.Core.LanguagePrimitives.HashCompare::GenericEqualityIntrinsic
Microsoft.FSharp.Core.LanguagePrimitives.HashCompare::GenericGreaterOrEqualIntrinsic
Microsoft.FSharp.Core.LanguagePrimitives.HashCompare::GenericGreaterThanIntrinsic
Microsoft.FSharp.Core.LanguagePrimitives.HashCompare::GenericLessThanIntrinsic
Microsoft.FSharp.Core.LanguagePrimitives.HashCompare::PhysicalEqualityIntrinsic
Microsoft.FSharp.Core.LanguagePrimitives::GenericEqualityComparer
Microsoft.FSharp.Core.LanguagePrimitives::GenericEqualityERComparer
Microsoft.FSharp.Core.LanguagePrimitives::ParseInt32
Microsoft.FSharp.Core.LanguagePrimitives::ParseInt64
Microsoft.FSharp.Core.LanguagePrimitives::ParseUInt32
Microsoft.FSharp.Core.LanguagePrimitives::ParseUInt64
Microsoft.FSharp.Core.Operators.OperatorIntrinsics::PowDouble
Microsoft.FSharp.Core.Operators.OperatorIntrinsics::RangeChar
Microsoft.FSharp.Core.Operators.OperatorIntrinsics::RangeDouble
Microsoft.FSharp.Core.Operators::Failure
System.Array::Copy
System.Char::Parse
System.Decimal::Parse
System.Decimal::op_Explicit
System.Decimal::op_GreaterThan
System.Decimal::op_LessThan
System.Globalization.CultureInfo::get_InvariantCulture
System.Type::GetTypeFromHandle

• error FABLE: method only accepts a single argument:

System.Single.parse only accepts a single argument
System.Double.parse only accepts a single argument

• hash of a string (i.e. hash "abc") for some reason results in an unconverted ILAsm(["I_call"])

• runtime error: "get_IsGenericType" is missing (in Reflection and Type tests only).

It's up to you if you want to proceed with adding the (highly experimental) optimize compiler switch before fixing some or all of the above.

I have submitted what I have so far to @forki 's PR so we should be on the same page once it's merged. Please note: OptimizedAssemblyContents has been renamed to GetOptimizedAssemblyContents() from review.

Thanks for the list @ncave! Most of the stuff is already there for other operations so it should doable. I'll give it a shot so we can move to the test in production phase as @forki suggests 😉

@ncave what do we do with this one?

@forki Nothing until #1266 is done. I think this one can be closed, even though I like the gifs.