Intentionally skip switch instructions during branch removal as the code doesn't handle it yet (throws NIE). #2889
Conversation
…ode doesn't handle it yet (throws NIE).
|
|
||
| // Common pattern generated by C# compiler in debug mode | ||
| if (i > 3 && GetConstantValue (FoldedInstructions[i - 3], out operand) && operand is int opint2 && IsPairedStlocLdloc (FoldedInstructions[i - 2], FoldedInstructions[i - 1])) { | ||
| if (i >= 3 && GetConstantValue (FoldedInstructions[i - 3], out operand) && operand is int opint2 && IsPairedStlocLdloc (FoldedInstructions[i - 2], FoldedInstructions[i - 1])) { |
There was a problem hiding this comment.
This fixes the case where the switch is the first thing in a method body - in which case the logic here would not kick in (now switch is disabled above, but if it's not it would not be recognized due to this).
There was a problem hiding this comment.
Sorry, I'm having trouble visualizing the kind of code this is matching. Could you write a comment with an example?
There was a problem hiding this comment.
This looks like it is a bug not just for switch statements but for any branching instruction we consider, right? I wonder if we can write a test for this fix where the compiler doesn't emit a nop as the first instruction.
@agocke I think it's for cases where the compiler emits code like this:
call bool C::get_ConstantProperty()
stloc.0
ldloc.0
brfalse.s IL_xxxx // or a switch
There was a problem hiding this comment.
Exactly - for some reason Roslyn always put the stloc/ldloc pair even in optimized build - in debug build there are actually two pairs like that one after another. The code would not handle the debug build at all right now, but that's for when we actually support switch.
|
|
||
| // Common pattern generated by C# compiler in debug mode | ||
| if (i > 3 && GetConstantValue (FoldedInstructions[i - 3], out operand) && operand is int opint2 && IsPairedStlocLdloc (FoldedInstructions[i - 2], FoldedInstructions[i - 1])) { | ||
| if (i >= 3 && GetConstantValue (FoldedInstructions[i - 3], out operand) && operand is int opint2 && IsPairedStlocLdloc (FoldedInstructions[i - 2], FoldedInstructions[i - 1])) { |
There was a problem hiding this comment.
This looks like it is a bug not just for switch statements but for any branching instruction we consider, right? I wonder if we can write a test for this fix where the compiler doesn't emit a nop as the first instruction.
@agocke I think it's for cases where the compiler emits code like this:
call bool C::get_ConstantProperty()
stloc.0
ldloc.0
brfalse.s IL_xxxx // or a switch
The linker bump in dotnet/linker#2889 should have fixed the issue. Closes dotnet#71848 .
The linker bump in dotnet/linker#2889 should have fixed the issue. Closes #71848 .
…ode doesn't handle it yet (throws NIE). (dotnet/linker#2889) Commit migrated from dotnet/linker@0872a5c
3 participants
This is more of a "workaround" to #2872
I created #2888 to track the work to add full support for switch instruction in branch removal.