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.

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

The F# compiler ignores MethodImplOptions.AggressiveInlining flag #1637

Closed
FrankNiemeyer opened this issue Oct 19, 2016 · 13 comments
Closed
Labels
Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Ready

Comments

@FrankNiemeyer
Copy link

The F# compiler ignores the System.Runtime.CompilerServices.MethodImplAttribute applied to methods/functions if the MethodImplOptions.AggressiveInlining flag is set and consequently doesn't emit the required IL metadata ("aggressiveinlining" method flag = 0x0100).

Descriptive Example

For example, compiling the following F# code

[<MethodImpl(MethodImplOptions.AggressiveInlining)>]
let inc x = x + 1

yields the following IL

.method public static int32  inc(int32 x) cil managed
{
    .maxstack  8
    IL_0000:  nop
    IL_0001:  ldarg.0
    IL_0002:  ldc.i4.1
    IL_0003:  add
    IL_0004:  ret
}

In contrast, the C# compiler translates

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int Inc(int x) => x + 1;

to

.method public hidebysig static int32  Inc(int32 x) cil managed aggressiveinlining
{     
    .maxstack  8
    IL_0000:  ldarg.0
    IL_0001:  ldc.i4.1
    IL_0002:  add
    IL_0003:  ret
}

generating the required aggressiveinling flag.

Further Experiments

The same behavior can be observed for MethodImplOptions.AggressiveInlining flags applied to methods or static methods of class types. Strangely enough, if applied to indexer get/set methods, the F# compiler doesn't ignore the attribute, but treats it like a custom attribute (instead of a pseudo custom attribute). For instance,

type Dummy =
    member self.Item
        with [<MethodImpl(MethodImplOptions.AggressiveInlining)>] get x = x + 1

becomes

.method public hidebysig specialname instance int32 get_Item(int32 x) cil managed
{
    .custom instance void [mscorlib]System.Runtime.CompilerServices.MethodImplAttribute::.ctor(valuetype [mscorlib]System.Runtime.CompilerServices.MethodImplOptions) = ( 01 00 00 01 00 00 00 00 ) 
    .maxstack  8
    IL_0000:  nop
    IL_0001:  ldarg.1
    IL_0002:  ldc.i4.1
    IL_0003:  add
    IL_0004:  ret
}

Known workarounds

None.

Related information

According to @kvb, PreserveSig, Synchronized, and NoInlining flags seem to be respected by the compiler, see this StackOverflow question.

Test environment:

  • Windows 10 Pro Build (14393.321)
  • .NET 4.6
  • F# 4.0
  • VisualStudio 2015
@kbattocchi
Copy link
Contributor

It might be beneficial to review the other pseudo-custom attributes (see e.g. the list here) in addition to MethodImpl to verify that there aren't other important pieces of metadata being inadvertently dropped.

@dsyme dsyme added Bug Area-Compiler Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. labels Oct 24, 2016
@buybackoff
Copy link
Contributor

Is this still the case for the latest F# shipped with VS2017?

@forki
Copy link
Contributor

forki commented May 30, 2017

AFAIK nobody has fixed this

@forki
Copy link
Contributor

forki commented May 30, 2017

I tried to find it in ECMA 335 but it's not there!?

@forki
Copy link
Contributor

forki commented May 30, 2017

WIP in #3154

@forki
Copy link
Contributor

forki commented May 30, 2017

mhm I just tried C#:

image

@forki
Copy link
Contributor

forki commented May 30, 2017

ok that was an issue with DotPeek. ILSpy is showing:

image

@forki
Copy link
Contributor

forki commented May 30, 2017

btw: it looks like noinlining is not respected as well.

@buybackoff
Copy link
Contributor

buybackoff commented Jun 23, 2017

I have created a simple workaround for this - just to rewrite IL after build and repack it with ilasm. It was simpler than #3154 for me. Such trick was used in corefxlab in the very early implementation of the S.R.CS.Unsafe package. All code is in this commit.

To add the attribute, one should add a custom attribute RewriteAIL next to MethodImpl(MethodImplOptions.AggressiveInlining) like here. Then after build a command started from a command line
disassembles a dll to IL, adds the aggressiveinlining attribute and repacks the modified IL code into a signed dll.

Tools and samples are in this folder.

I couldn't integrate it with build automatically, but after manually calling rewrite scripts as admin the command dotnet pack with --no-build works fine.

This gives small but stable c.5% performance improvement for the hottest method in the library.
image

@dsyme dsyme added the Ready label Jun 23, 2017
@dsyme
Copy link
Contributor

dsyme commented Jun 23, 2017

@buybackoff The fix #3154 is now ready

@buybackoff
Copy link
Contributor

@dsyme Thanks a lot! When it will be available? Several percents are not a game changer but they are real and free 😄

@dsyme
Copy link
Contributor

dsyme commented Jun 23, 2017

@buybackoff probably not until Visual Studio 2017 Update 4, unless you're using some other packaging of the F# compiler. Maybe Visual Studio 2017 Update 3 but that depends on what the Microsoft ship people decide.

@dsyme
Copy link
Contributor

dsyme commented Jun 26, 2017

Merged

@dsyme dsyme closed this as completed Jun 26, 2017
dsyme pushed a commit that referenced this issue Jun 26, 2017
…3154)

* Implement `MethodImplOptions.AggressiveInlining` flag - fixes #1637

* More inlining

* remove change to TAST

* fix build

* add test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Ready
Projects
None yet
Development

No branches or pull requests

5 participants