Fsharp: Nop IL instructions in release code

[Szer]

FSC generates nop instructions after void calls because of this
https://github.com/fsharp/fsharp/blob/6819e1c769269edefcea2263c98f993e90b623e2/src/fsharp/IlxGen.fs#L2702

But he also continues to do so even in release configuration!

Repro steps

1. fsproj:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Program.fs" />
  </ItemGroup>

</Project>

1. Program.fs

module Main

type Foo = struct
    end

and [<AbstractClass>] Bar() =    
    abstract bar: foo: byref<Foo> -> unit

let bar = 
    { new Bar() with
        override x.bar (foo: byref<Foo>) =
        x.bar(&foo) }

[<EntryPoint>]
let main _ =    
    let mutable a = new Foo()
    bar.bar(&a)
    0

1. dotnet run -c Release

Attached project
ConsoleApp18.zip

Expected behavior

There shouldn't be nop instructions in IL (this is Release config)
And as neat side effect it should be JIT-friendly to be optimized by JIT and run forever without Stack Overflow. (callvirt just before ret)

bar.bar should be like this:

Actual behavior

Nop instruction after every void call
JIT can't optimize this pattern:

callvirt ...
nop
ret

and everything fails with SO

bar.bar looks like this:

Known workarounds

Adding <DebugType>ByDesign</DebugType> fixes the issue.
Why? Glad you asked.

Because SetDebugSwitch method fails horribly on this line with ByDesign switch (also with none but ByDesign is better)
https://github.com/fsharp/fsharp/blob/6819e1c769269edefcea2263c98f993e90b623e2/src/fsharp/CompileOptions.fs#L480
and doesn't execute this line
tcConfigB.debuginfo <- s = OptionSwitch.On

This fixes everything (but breaks something else). nop instructions won't be emited.

Related information

Provide any related information

• Win10
• .NET Core SDK 2.1.502

[forki]

Hilarious workaround 😂

Any chance you could send 2 PRs?

[Szer]

@forki I could, but I honestly not sure which way to fix it is better.

Fix 1:

Remove these 2 lines as a whole. C# seems not care too much about nop after void calls.
https://github.com/fsharp/fsharp/blob/6819e1c769269edefcea2263c98f993e90b623e2/src/fsharp/IlxGen.fs#L2704

Fix 2:
Make same 2 lines aware about <Optimize> flag and forget about debug symbols presence. Because we could have PDBs even in release configuration, but nop should be removed only (?) in optimized builds

[ForNeVeR]

Does C# add nop there in debug configuration? If so, then it's probably a good idea to add it, but only with optimization disabled. Looks like the nop-generating code was added without too much reason, just to mirror C# behavior.

[Szer]

@ForNeVeR C# indeed generates nop in this particular case for debug builds:

using System;

namespace ConsoleApp19
{
    struct Foo { }

    abstract class Bar
    {
        public abstract void bar(ref Foo foo);
    }

    class RealBar : Bar
    {
        public override void bar(ref Foo foo)
        {
            bar(ref foo);
        }
    }

    class Program
    {
        static int Main(string[] args)
        {
            var a = new Foo();
            var bar = new RealBar();
            bar.bar(ref a);
            return 0;
        }
    }
}

Debug:

Release:

[0x53A]

AFAIK the reason for the nops is so that you can set a breakpoint on lines that otherwise dont map to an instruction (e.g. closing braces)

[forki]

question: is there any benefit from any nop in optimized builds? Shouldn't they be eliminated everywhere?

[Szer]

question: is there any benefit from any nop in optimized builds?

no benefits. They are useful for placing breakpoints. And because they are wasting cycles they should be eliminated from optimized builds.

Shouldn't they be eliminated everywhere?

they should

[abelbraaksma]

@forki, no, not in IL, but in native code opcodes it's common, and usually used to machine-word-align instructions or improve pipelining hints to the CPU. There maybe other cases, but in IL itself I doubt it's useful, let alone that it needs instruction alignment.

[cartermp]

Yeah, this is more of a bug that this happens. It should only be for debug.

[dsyme]

I'm in two minds about this.

1. The tailcall is not guaranteed in this situation, because this is passing a byref parameter, and we never take tailcalls for calls involving byrefs as they may come from the current stack frame.

2. In theory the nop should be harmless for performance

3. Removing the nop may plausibly degrade something

However in practice there appear to be JIT optimizations which are sensitive to the presence of nop, so I guess we can remove them from release code on that basis.

[zpodlovics]

Based on the earlier comment on importer it seems that nops only imported in debug code (search CEE_NOP in the code): https://github.com/dotnet/runtime/blob/857b2db411741ee57bfe24da58f77e6e124aa110/src/coreclr/src/jit/importer.cpp#L16345