ILLink is removing branch in System.SR.GetResourceString

[eerhardt]

In the dotnet/runtime libraries, we have the following code in System.SR:

        // This method is used to decide if we need to append the exception message parameters to the message when calling SR.Format.
        // by default it returns false.
        // Native code generators can replace the value this returns based on user input at the time of native code generation.
        // Marked as NoInlining because if this is used in an AoT compiled app that is not compiled into a single file, the user
        // could compile each module with a different setting for this. We want to make sure there's a consistent behavior
        // that doesn't depend on which native module this method got inlined into.
        [MethodImpl(MethodImplOptions.NoInlining)]
        private static bool UsingResourceKeys() => false;

        internal static string GetResourceString(string resourceKey, string? defaultString = null)
        {
            if (UsingResourceKeys())
            {
                return defaultString ?? resourceKey;
            }

            // do the resource look-up

In the 3.1 libraries, the above code would produce the following IL:

.method assembly hidebysig static string 
        GetResourceString(string resourceKey,
                          [opt] string defaultString) cil managed
{
  .param [2] = nullref
  // Code size       49 (0x31)
  .maxstack  2
  .locals (string V_0)
  IL_0000:  call       bool System.SR::UsingResourceKeys()
  IL_0005:  brfalse.s  IL_000e
  IL_0007:  ldarg.1
  IL_0008:  dup
  IL_0009:  brtrue.s   IL_000d
  IL_000b:  pop
  IL_000c:  ldarg.0
  IL_000d:  ret
  IL_000e:  ldnull
  IL_000f:  stloc.0
  .try
  {
    IL_0010:  call       class [System.Resources.ResourceManager]System.Resources.ResourceManager System.SR::get_ResourceManager()

However, in the latest 5.0 builds, this is the IL:

.method assembly hidebysig static string 
        GetResourceString(string resourceKey,
                          [opt] string defaultString) cil managed
{
  .param [2] = nullref
  // Code size       42 (0x2a)
  .maxstack  2
  .locals (string V_0)
  IL_0000:  call       bool System.SR::UsingResourceKeys()
  IL_0005:  brfalse.s  IL_0007
  IL_0007:  ldnull
  IL_0008:  stloc.0
  .try
  {
    IL_0009:  call       class [System.Runtime]System.Resources.ResourceManager System.SR::get_ResourceManager()

Note that we are still issuing the call to System.SR::UsingResourceKeys(), but we are stripping out the branch code that will return defaultString ?? resourceKey;.

The above SR code is what we have in place for being able to remove resource strings on UWP. I was attempting to flip the hard-coded bool to true to test out how much size we could save if we used the same approach. However, since ILLink is running during our library build (before we try trimming for size in the context of an app), it is causing the pattern to no longer work.

Maybe ILLink should respect the [MethodImpl(MethodImplOptions.NoInlining)] attribute when it looks for constant method bodies, and not try to fold them?

cc @marek-safar @vitek-karas @MichalStrehovsky @sbomer

[marek-safar]

That's by design where the default constant propagation kicks-in. You can use the following to disable

• disable the optimization for this assembly
• add IntrinsicAttribute to the method for the linker to not analyze the body
• change the body to return static field instead of constant

Maybe ILLink should respect the [MethodImpl(MethodImplOptions.NoInlining)] attribute when it looks for constant method bodies, and not try to fold them?

We thought about it but decided not to as the [MethodImpl(MethodImplOptions.NoInlining)] should be there for a bit different purposes.

[eerhardt]

add IntrinsicAttribute to the method for the linker to not analyze the body

We thought about it but decided not to as the [MethodImpl(MethodImplOptions.NoInlining)] should be there for a bit different purposes.

I don't know if the IntrinsicAttribute is the correct one to mark this as "don't inspect my body". That one is a totally different purpose - it means the runtime is going to replace the body of the method.

https://github.com/dotnet/runtime/blob/2c87db6b2ec2d4a7f0c20b33378b0f7aaeccea26/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/IntrinsicAttribute.cs#L7-L9

    // Calls to methods or references to fields marked with this attribute may be replaced at
    // some call sites with jit intrinsic expansions.
    // Types marked with this attribute may be specially treated by the runtime/compiler.

To me, marking [MethodImpl(MethodImplOptions.NoInlining)] makes more sense because:

1. The description of this attribute fits with this scenario: The method cannot be inlined. Inlining is an optimization by which a method call is replaced with the method body. That's exactly what the linker is trying to do here - inline the constant result.
2. MethodImpl is public, so people outside of CoreLib can use it. System.Console can't use the IntrinsicAttribute as it exists today, and neither can 3rd parties.

[eerhardt]

A separate issue is that we should be removing the call to the method UsingResourceKeys, but we aren't. I've logged #1113 specifically for that.

[marek-safar]

I was attempting to flip the hard-coded bool to true to test out how much size we could save if we used the same approach

My recommendation was for this your request. If you ask for a permanent solution then IntrinsicAttribute is not the way forward.

That's exactly what the linker is trying to do here - inline the constant result.

That's exactly what linker is NOT doing. The linker does not inline the call as you can see in the output IL.

[vitek-karas]

Per offline discussion - some more reasons as to why is this happening: #1106 (comment)

[MichalStrehovsky]

In what scenarios is inlining the return value of a NoInlining method beneficial? Do we have a use case where this makes a meaningful difference (or any difference at all)?

[marek-safar]

We could fix linker to ignore NoInlining as well but no inlining should not alter program behaviour/result which is what is happening in this case. I still think the framework code is abusing NoInlinig in this case for something that IntrinsicAttribute is used elsewhere in the libraries for.

[MichalStrehovsky]

We could mark it Intrinsic, but Intrinsic is typically only used in CoreLib. This affects all SR classes within framework (so e.g. System.Linq would need to carry the definition of the IntrinsicsAttribute so that it can put it on the method - because the attribute is not public). I'm not opposed to doing that, but I expect some pushback from the libraries team if we were to do that.

[vargaz]

This really constraints the ability of products to use substitutions since by the time they receive the IL, it's already substituted.
Also, the linker does inline this case, since it uses the result value of the method to rewrite the caller. 'do not inline' usually includes 'do not do cross function optimization' in most languages, like in C/C++. So I'd suggest the linker should do the same.

[vargaz]

It would also be nice to be able to turn off this behavior by an option and just do the substitutions in the xml file.

[marek-safar]

It would also be nice to be able to turn off this behavior by an option and just do the substitutions in the xml file.

That's not necessary. We could simply not to apply the attribute for targets/RIDs where we don't want the "dynamic" behaviour so the substitutions would still kick-in.

[vargaz]

Having the ability to turn off optimizations is a very useful feature, it can give users a workaround if they run into a bug with the optimization.

[marek-safar]

There is the switch --disable-opt NAME [ASM] to turn it off globally or on assembly-level just don't think it's useful for this scenario