Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix incorrect switch temp lcl type #13487

Merged
merged 1 commit into from
Aug 22, 2017

Conversation

mikedn
Copy link

@mikedn mikedn commented Aug 19, 2017

Fixes #13486

@mikedn
Copy link
Author

mikedn commented Aug 19, 2017

x64 jit-diff fx:

Total bytes of diff: -7 (0.00% of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
           2 : System.Diagnostics.Process.dasm (0.00% of base)
Top file improvements by size (bytes):
          -6 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          -2 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
          -1 : Microsoft.CodeAnalysis.dasm (0.00% of base)
4 total files with size differences (3 improved, 1 regressed), 75 unchanged.
Top method regessions by size (bytes):
           2 : System.Diagnostics.Process.dasm - Process:GetShellError(long):int:this
           1 : Microsoft.CodeAnalysis.CSharp.dasm - BaseTypeAnalysis:IsManagedTypeHelper(ref):ubyte
           1 : Microsoft.CodeAnalysis.CSharp.dasm - SourceNamespaceSymbol:BuildSymbol(ref,ref):ref:this
           1 : Microsoft.CodeAnalysis.dasm - SwitchConstantValueHelper:IsValidSwitchCaseLabelConstant(ref):bool
Top method improvements by size (bytes):
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - Lexer:ScanIdentifier_FastPath(byref):bool:this
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - ConversionEasyOut:TypeToIndex(ref):int
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - BinopEasyOut:TypeToIndex(ref):struct
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - UnopEasyOut:TypeToIndex(ref):struct
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - VarianceSafety:IsVarianceUnsafe(ref,bool,bool,ref,ref,ref,ref):bool (2 methods)
16 total methods with size differences (12 improved, 4 regressed), 66256 unchanged.

x86 jit-diff fx:

Total bytes of diff: 11 (0.00% of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
          18 : System.Private.CoreLib.dasm (0.00% of base)
           1 : Microsoft.CodeAnalysis.dasm (0.00% of base)
Top file improvements by size (bytes):
          -5 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          -3 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.00% of base)
4 total files with size differences (2 improved, 2 regressed), 75 unchanged.
Top method regessions by size (bytes):
          18 : System.Private.CoreLib.dasm - EventSource:GetHelperCallFirstArg(ref):int
           2 : Microsoft.CodeAnalysis.CSharp.dasm - ConversionEasyOut:TypeToIndex(ref):int
           1 : Microsoft.CodeAnalysis.CSharp.dasm - BaseTypeAnalysis:IsManagedTypeHelper(ref):ubyte
           1 : Microsoft.CodeAnalysis.CSharp.dasm - SourceNamespaceSymbol:BuildSymbol(ref,ref):ref:this
           1 : Microsoft.CodeAnalysis.dasm - SwitchConstantValueHelper:IsValidSwitchCaseLabelConstant(ref):bool
Top method improvements by size (bytes):
          -3 : Microsoft.CodeAnalysis.CSharp.dasm - BinderFactoryVisitor:VisitXmlNameAttribute(ref):ref:this
          -2 : Microsoft.CodeAnalysis.CSharp.dasm - DocumentationCommentCompiler:BindName(ref,ref,ref,byref,byref,ref)
          -2 : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:ReportOverloadResolutionFailureAndProduceBoundNode(ref,int,ref,struct,ref,struct,struct,ref,ref,ref,ref,bool,ref,ref,ref):ref:this
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - Lexer:ScanIdentifier_FastPath(byref):bool:this
          -1 : Microsoft.CodeAnalysis.CSharp.dasm - VarianceSafety:IsVarianceUnsafe(ref,bool,bool,ref,ref,ref,ref):bool (2 methods)
13 total methods with size differences (8 improved, 5 regressed), 66198 unchanged.

Most of the diffs look like:

        movsx    eax, al
-       cmp      al, 38
+       cmp      eax, 38
        ja       SHORT G_M33493_IG08

The 18 byte regression from EventSource:GetHelperCallFirstArg has to do with register allocation choices. While unrelated to this it looks kind of interesting so here's a full diff: https://gist.github.com/mikedn/15e61328a8fe0812268125af04fa2b89

@mikedn
Copy link
Author

mikedn commented Aug 19, 2017

@dotnet-bot test Tizen armel Cross Debug Build

@mikedn mikedn changed the title [WIP] Fix incorrect switch temp lcl type Fix incorrect switch temp lcl type Aug 20, 2017
@mikedn
Copy link
Author

mikedn commented Aug 20, 2017

It's probably worth pointing out that on x64 this bug results in silent bad code generation. For example the JIT generated:

       4883F802             cmp      rax, 2
       7748                 ja       SHORT G_M55888_IG08
       488D0D65000000       lea      rcx, [reloc @RWD00]
       8B0C81               mov      ecx, dword ptr [rcx+4*rax]

instead of the correct:

       83F802               cmp      eax, 2
       774A                 ja       SHORT G_M55888_IG08
       8BC0                 mov      eax, eax
       488D0D64000000       lea      rcx, [reloc @RWD00]
       8B0C81               mov      ecx, dword ptr [rcx+4*rax]

But it's unlikely to hit this in practice, at least in C# where this pattern occurs when switching on longs and the C# compiler adds its own long compare before the switch:

G_M55888_IG02:
       488BC1               mov      rax, rcx
       4883F802             cmp      rax, 2
       774E                 ja       SHORT G_M55888_IG08
G_M55888_IG03:
; yeah, it's wrong if the upper 32 bits aren't 0 but that will never happen due to the above check
       4883F802             cmp      rax, 2 
       7748                 ja       SHORT G_M55888_IG08
       488D0D65000000       lea      rcx, [reloc @RWD00]
       8B0C81               mov      ecx, dword ptr [rcx+4*rax]

@mazong1123
Copy link

@dotnet-bot test Ubuntu x64 corefx_baseline
@dotnet-bot test Windows_NT x86 corefx_baseline

Just make sure this PR fixes the CI failure...

Copy link

@JosephTremoulet JosephTremoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing this.

@JosephTremoulet JosephTremoulet merged commit 10c89f4 into dotnet:master Aug 22, 2017
@karelz karelz modified the milestone: 2.1.0 Aug 28, 2017
@mikedn mikedn deleted the switch-temp-type branch December 16, 2017 09:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants