Add InstructionEncoder.Switch to enable emitting switch instructions with LabelHandles

[SeeminglyScience]

Background and Motivation

It doesn't look like there's a way to use InstructionEncoder.DefineLabel's LabelHandles with a switch instruction. I'm mainly asking if there is a way to do it that I'm missing, but if not here's a basic API proposal to enable it.

Proposed API

namespace System.Reflection.Metadata.Ecma335
{
    public readonly struct InstructionEncoder
    {
+       public void Switch(params LabelHandle[] label);
    }
}

Usage Examples

var ie = new InstructionEncoder(new BlobBuilder(), new ControlFlowBuilder());
var case0 = ie.DefineLabel();
var case1 = ie.DefineLabel();
var case2 = ie.DefineLabel();
var afterCases = ie.DefineLabel();

ie.OpCode(ILOpCode.Ldc_i4_0);
ie.Switch(case0, case1, case2);
ie.Branch(ILOpCode.Br_s, afterCases);

ie.MarkLabel(case0);
ie.OpCode(ILOpCode.Ldc_i4_0);
ie.OpCode(ILOpCode.Ret);

ie.MarkLabel(case1);
ie.OpCode(ILOpCode.Ldc_i4_1);
ie.OpCode(ILOpCode.Ret);

ie.MarkLabel(case2);
ie.OpCode(ILOpCode.Ldc_i4_2);
ie.OpCode(ILOpCode.Ret);

ie.MarkLabel(afterCases);
ie.OpCode(ILOpCode.Ldc_i4_s);
ie.CodeBuilder.WriteCompressedSignedInteger(10);
ie.OpCode(ILOpCode.Ret);

new MethodBodyStreamEncoder(new BlobBuilder())
    .AddMethodBody(ie, maxStack: 8, localVariablesSignature: default, MethodBodyAttributes.None);

Alternative Designs

Originally this proposal was based on emitting the label by itself:

namespace System.Reflection.Metadata.Ecma335
{
    public readonly struct InstructionEncoder
    {
+       public void Label(LabelHandle label);
    }
}

This won't work because the target offset is based on the end of the switch instruction, not the offset of the label.

Also for it to work with switch like intended, it would need to only emit int32 offsets. That's not super clear from the name and would generate bad IL if someone were to do OpCode(ILOpCode.Br_s); Label(label);, even if it were based on the label's offset.

Risks

[teo-tsirpanis]

I think it should also have an overload for IReadOnlyCollection<LabelHandle>, to allow high-performance apps to reuse a List<LabelHandle>.

[teo-tsirpanis]

I have refactored the relevant SRM code to easily support emitting switch. Is there anything that prevents the proposal from going forward?

[buyaa-n]

@tmat what do you think about this proposal? The main question is there a way to use InstructionEncoder.DefineLabel's LabelHandles with a switch instruction

[August-Alm]

Any updates? I would really like to see this proposal implemented! (I'd be very thankful if someone could provide a temporary workaround.)

[SeeminglyScience]

Any updates? I would really like to see this proposal implemented! (I'd be very thankful if someone could provide a temporary workaround.)

This is probably not the temporary workaround you're hoping for, but what I did was basically rip out and modify their flow control builder code. Here's the PR where I did that if you want some ideas SeeminglyScience/ILAssembler#19

[teo-tsirpanis]

@buyaa-n, @tmat can this move forward? And @SeeminglyScience can you also propose an overload that takes an IReadOnlyCollection<LabelHandle>?

[SeeminglyScience]

And @SeeminglyScience can you also propose an overload that takes an IReadOnlyCollection<LabelHandle>?

I tried to keep the proposal as close to other APIs in the library as possible. If we were adding overloads for performance, I'd rather see ReadOnlySpan<LabelHandle> added (but that also is not present in the API surface today). For now, I'm going to keep the proposal as is.

[teo-tsirpanis]

Yeah, spans would not fit because they are nowhere else used in this library. IReadOnlyCollection would be the next most memory-efficient solution (in a library where memory efficiency is a theme), but let's wait for the API owners' opinion.

[tmat]

We could add the proposed API that takes LabelHandle[] but a non-allocating version like following would be more aligned with the spirit of the encoder APIs.

public readonly struct SwitchInstructionEncoder
{
   private readonly ControlFlowBuilder _branchBuilder;
   private readonly BlobBuilder _codeBuilder;
   private readonly int _instructionEndOffset;

   internal SwitchInstructionEncoder(ControlFlowBuilder branchBuilder, BlobBuilder codeBuilder, int instructionEndOffset) { ... }

   public void Branch(Label label)
     => _branchBuilder.AddBranch(_instructionEndOffset, label, _codeBuilder);
}

public readonly struct InstructionEncoder
{
   public SwitchInstructionEncoder Switch(int branchCount)
   {
      var branchBuilder = GetBranchBuilder();
      OpCode(ILOpCodes.Switch);      
      CodeBuilder.WriteInt32(branchCount);
      return new SwitchInstructionEncoder(branchBuilder, CodeBuilder, Offset + branchCount * sizeof(uint));
   }
}

[bartonjs]

Video

• We went with the alternative proposal with SwitchInstructionEncoder instead of the params array, since it felt more consistent with existing paradigms.

namespace System.Reflection.Metadata.Ecma335
{
    public readonly struct SwitchInstructionEncoder
    {
        public void Branch(LabelHandle label);
    }

    public partial struct InstructionEncoder
    {
        public SwitchInstructionEncoder Switch(int branchCount);
    }
}

[teo-tsirpanis]

I will do it in a couple of months in time for 8.0.0, if someone else wants to do it, feel free to.

[August-Alm]

The original proposal had a clear usage example but the approved API leaves the intended functionality implicit. Am I correct in understanding the approved proposal as supposed to work in such a way that the originally suggested method is equivalent to the following InstructionEncoder method?

public void SwitchOfLabelHandles(params LabelHandle[] labels)
{
    var switchEncoder = Switch(labels.Length);
    foreach (var label in labels)
    {
        switchEncoder.Branch(label);
    }
}

[teo-tsirpanis]

@August-Alm yes.