[API review] Add MethodImplOptions.AggressiveOptimization

[kouvel]

Add MethodImplOptions.AggressiveOptimization as a counterpart to MethodImplOptions.NoOptimization. NoOptimization would override AggressiveOptimization.

    [Flags]
    public enum MethodImplOptions
    {
        // Unmanaged = 0x0004,
        // NoInlining = 0x0008,
        // ForwardRef = 0x0010,
        // Synchronized = 0x0020,
        // NoOptimization = 0x0040,
        // PreserveSig = 0x0080,
        // AggressiveInlining = 0x0100,
        AggressiveOptimization = 0x0200,
        // InternalCall = 0x1000
    }

This would also add a new keyword aggressiveoptimization to IL ASM:

  .method private hidebysig static void  Main() cil managed aggressiveoptimization

The flag could be used in a MethodImplAttribute to indicate that the method contains hot code:

• For tiering, it could cause tier 1 JIT to be used right away for the method, as a workaround for https://github.com/dotnet/coreclr/issues/19751
  • For this reason, we would also like to add this API to 2.2, as it is currently planned to enable tiering by default in 2.2 for release
• It could allow the JIT to spend more JIT time to generate better code, such as inlining more aggressively into the function

CC @noahfalk @vancem @dotnet/jit-contrib

[MichalStrehovsky]

Note that changing this enum is a file format change, not just an API change. ECMA-335 specifies the valid values for this enumeration in section II.22.26 and also says values out of the range of allowed values should be treated as error.

For example, this new value won't roundtrip through ILDASM/ILASM without revving IL syntax.

[kouvel]

I see. I have local changes to update ildasm/ilasm. What is the process to propose the change to the spec?

[kouvel]

Also is there an IL version (corresponding to spec version) that should be revved and taken into account for providing errors?

[vancem]

I think the language in the spec that says unspecified values are errors should be modified. As a general principle the spec should not prohibit extensions without good reason. If the spec is still accepting updates, we can add that to it, but we should not block on it.

Ideally the situation should be ILASM should have syntax for values it does not know about (e.g hex numbers), and should encode them so that they can be round-tripped. We should fix ILASM/DASM to do this.

I am interested in knowing what the ILASM/DASM tools do (does it strip it? That is not so bad in this case. This will not be the last time we will want another bit to 'hang' on a method. If MethodImplOptions is going to be problematic because tools did fragile things, we may need to use a 'real' custom attribute instead. First we have to find out how bad it is in actual practices (any reader tools blow up?).

[kouvel]

I modified a binary to set the new enum value on Main. The baseline ILDASM without changes is stripping the unknown value and not throwing:

  .method private hidebysig static void  Main() cil managed

The modified ILDASM is recognizing the new value:

  .method private hidebysig static void  Main() cil managed aggressiveoptimization

The baseline CoreCLR is not throwing an exception. It looks like there is a flag check when using System.Reflection.Emit.TypeBuilder.DefineCustomAttribute() to throw for invalid unknown flags, which is probably fine.

[kouvel]

ILSpy extension in VS is showing the value like this:

[MethodImpl((MethodImplOptions)512)]
private static void Main()

[vancem]

@kouvel, how did you fix ILASM? did you invent a new keyword? That is fine, but in addition it would be nice if we can make ILASM syntax that is like ILSpy's (round tripping works for anything in the future without modification).

I don't have a lot of experience knowing what kind of things break when IL is updated. I think we should still proceed, but if anyone knows of things that might break (e.g. CCI and tools based on it), a heads up would be appropriate.

@jkotas FYI.

[kouvel]

Yea I added the new keyword. Maybe something like methodimploptions(512) would be appropriate for generic handling. Are there any tools other than ilasm that parse ildasm's output or IL-source?

[vancem]

I am sure there are a bunch of ad-hoc tools that parse ILASM, but most are probably pretty robust.

The current syntax looks in ILDASM is the following (where 'noinlining' is a MethodImplption).

  .method public hidebysig static void  Main(string[] args) cil managed noinlining

Thus the syntax is simpy a identifier that follows the parameters. While your suggestion of allowing one of these to be methodimploption(number), is reasonable, I worry that it woud 'trip up' simplfied parsers (e.g. regular expressions (.*)). Thus I think it would be slightly better to have code that makes up 'generic' value identifiers. For example methodImplOption_200 (where 200 is the hex value.

Ideally we would do this for other bits that have special sytnax in ILDASM, but I am OK with just doing MethodImplOptions for now (although if people can think of ones that are likely to be augmented, we should do those proactively).

[kouvel]

I guess the new method of specification should be in addition to the keywords for back-compat and should be preferred over the keywords if we add new bits in the future without new keywords.

[jkotas]

This is not the first time we are allocating a new bit here. I do not recall adding a new keyword for a new bit here ever being a problem.

should be preferred over the keywords

The keywords are there to make the text human readable. I do not think we want to be encouraging numbers that you need to lookup somewhere else.

[AndyAyersMS]

Any thoughts on what should happen if a method marked for AggressiveOptimization is also an inline candidate?

My inclination would be to not inline such methods unless the root method was also so marked.

[vancem]

I agree with Andy's inclination.

What I like about AgressiveOptimization is that the guidance is straightfoward.

I believe that the guidance for AgressiveOptimization for a method M is the method M is hot (e.g. uses a few % or more of the CPU) in at least one important scenario.

Since a method can be called from many places (some of which are not hot), you should not assume that EVERY place the method is used deserves to be optimized agressvely. Certainly if its caller is marked for AgressiveOptimzation then it should be optimized, but the JIT is allowed to use additional heuristics.

It is another question whether a method is so small and likely to expose more optimization (e.g. a field fetch), that it should be optimized in almost all contexts (the only exception is that you are REALLY trying to save JIT time).

Can someone from the JIT team comment on what 'AgressiveInlining' means? In do you mark the callee or the caller (I am assuming it is the callee). Note AgressiveOptimization does not work like that (if you apply AgressiveOptimziation to M it is not likley M will be inlined, it is likey that M will inline methods it calls.

For what it is worth...

[CarolEidt]

The spec is not a static thing - even though we are not currently working on updates to the spec, we should plan on having that opportunity in the future.

I like the idea of extending ildasm to flexibly handle as-yet-undefined values, and however we decide to expose them in ildasm we should modify ilasm to interpret them accordingly.

It is another question whether a method is so small and likely to expose more optimization (e.g. a field fetch), that it should be optimized in almost all contexts (the only exception is that you are REALLY trying to save JIT time).

I would think that such a method would be a better candidate for AggressiveInlining than for AggressiveOptimization, but if the latter were specified I think that the approach that @AndyAyersMS suggests would be appropriate - only inline if the caller has AggressiveOptimization.

Can someone from the JIT team comment on what 'AgressiveInlining' means? In do you mark the callee or the caller (I am assuming it is the callee).

You're correct, it is the callee. But I would mildly disagree with this statement:

Note AgressiveOptimization does not work like that (if you apply AgressiveOptimziation to M it is not likley M will be inlined, it is likey that M will inline methods it calls.

In some sense if you apply AggressiveOptimization to M, then it probably makes it more likely that it will be inlined into methods that are also marked AggressiveOptimization, which would allow you to tweak where you really want it inlined into.

Of course, this has the downside of presenting more knobs that can potentially be abused, but I think it's all to the general good.