Skip to content

Remove popped trees more aggressively.#31677

Merged
erozenfeld merged 1 commit intodotnet:masterfrom
erozenfeld:DiscardPoppedNodesMoreAggressively
Feb 3, 2020
Merged

Remove popped trees more aggressively.#31677
erozenfeld merged 1 commit intodotnet:masterfrom
erozenfeld:DiscardPoppedNodesMoreAggressively

Conversation

@erozenfeld
Copy link
Member

@erozenfeld erozenfeld commented Feb 3, 2020 •
edited
Loading

When processing CEE_POP the importer sometimes created trees
like
ASG(tmp1, expr)
followed by
COMMA(ADDR(tmp1), NOP))
and that was causing some CQ issues.

LocalAddressVisitor was then marking tmp1 as address-exposed,
morph adds a GTF_GLOB_REF flag for all address-exposed locals,
and we never get rid of the dead ASG(tmp1, expr). What's worse, we
then were adding a zero-initialization for tmp1 in the prolog.

The fix here is to create a NOP instead of a COMMA if there are
no side effects in the first operand of the COMMA.

This addresses the first example in #2325.

@erozenfeld
Remove popped trees more aggressively.
dba41e0 
When processing CEE_POP the importer sometimes created trees
like
ASG(tmp1, expr)
followed by
COMMA(ADDR(tmp1), NOP))
and that was causing some CQ issues.

LocalAddressVisitor was then marking tmp1 as address-exposed
and we never get rid of the dead ASG(tmp1, expr). What's worse, we
then were adding a zero-initialization for tmp1 in the prolog.

The fix here is to create a NOP instead of a COMMA is there are
no side-effects in the first operand of the COMMA.

This addresses the first example in dotnet#2325.
@erozenfeld
Copy link
Member Author

erozenfeld commented Feb 3, 2020

x64 framework PMI diffs:

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for  default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: -3886 (-0.01% of base)
    diff is an improvement.
Top file improvements (bytes):
       -2110 : Microsoft.CodeAnalysis.CSharp.dasm (-0.05% of base)
       -1127 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.02% of base)
        -138 : System.Threading.Tasks.Dataflow.dasm (-0.02% of base)
        -117 : Microsoft.CodeAnalysis.dasm (-0.01% of base)
         -83 : System.Data.Common.dasm (-0.01% of base)
         -79 : System.Private.CoreLib.dasm (-0.00% of base)
         -40 : System.Net.Http.dasm (-0.01% of base)
         -37 : System.Private.Xml.dasm (-0.00% of base)
         -31 : System.Net.Primitives.dasm (-0.05% of base)
         -24 : System.Linq.Expressions.dasm (-0.00% of base)
         -24 : System.Net.NetworkInformation.dasm (-0.06% of base)
         -24 : System.Runtime.WindowsRuntime.dasm (-0.02% of base)
         -20 : System.Runtime.Caching.dasm (-0.03% of base)
         -10 : System.Data.Odbc.dasm (-0.01% of base)
         -10 : System.Data.SqlClient.dasm (-0.00% of base)
          -8 : System.Transactions.Local.dasm (-0.01% of base)
          -4 : System.Net.Sockets.dasm (-0.00% of base)
17 total files with Code Size differences (17 improved, 0 regressed), 148 unchanged.
Top method improvements (bytes):
        -101 (-3.51% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseNamespaceBody(byref,byref,byref,ushort):this
         -83 (-28.82% of base) : System.Data.Common.dasm - RBTree`1:RemoveAt(int):this (7 methods)
         -70 (-3.61% of base) : System.Threading.Tasks.Dataflow.dasm - SourceObservable`1:Unsubscribe(IObserver`1):this (7 methods)
         -68 (-1.32% of base) : System.Threading.Tasks.Dataflow.dasm - ObserversState:NotifyObserversOfCompletion(Exception):this (7 methods)
         -65 (-7.83% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseQueryBody():QueryBodySyntax:this
         -63 (-3.99% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseInterpolatedStringToken():ExpressionSyntax:this
         -63 (-4.50% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - NamedTypeSymbol:AtomicStoreArrayAndDiagnostics(byref,ImmutableArray`1,DiagnosticBag):this (7 methods)
         -60 (-5.46% of base) : Microsoft.CodeAnalysis.dasm - SwitchIntegralJumpTableEmitter:GenerateSwitchBuckets(int,int):ImmutableArray`1:this
         -58 (-59.79% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - AnonymousTypeDescriptor:AssertGood():this
         -56 (-3.19% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SourceModuleSymbol:GetAllDeclarationErrors(CancellationToken,byref):ImmutableArray`1:this
         -51 (-6.30% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicDataFlowAnalysis:AnalyzeReadWrite():this
         -47 (-4.10% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseBaseList():BaseListSyntax:this
         -44 (-3.06% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseParameterList(byref,SeparatedSyntaxListBuilder`1,byref,ushort,ushort,bool,bool,bool):this
         -44 (-2.85% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseTypeParameterConstraintClause():TypeParameterConstraintClauseSyntax:this
         -42 (-5.33% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseTypeArgumentList(byref,SeparatedSyntaxListBuilder`1,byref):this
         -41 (-3.79% of base) : Microsoft.CodeAnalysis.CSharp.dasm - DocumentationCommentParser:ParseCrefTypeSuffix(TypeSyntax):TypeSyntax:this
         -41 (-12.58% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SourceMethodSymbol:GenerateDeclarationErrors(CancellationToken):this
         -40 (-42.55% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AnonymousTypeDescriptor:AssertIsGood():this
         -33 (-3.10% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseArgumentList(byref,byref,byref,ushort,ushort):this
         -33 (-2.56% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseArrayRankSpecifier(bool,bool,byref):ArrayRankSpecifierSyntax:this
Top method improvements (percentages):
         -58 (-59.79% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - AnonymousTypeDescriptor:AssertGood():this
         -40 (-42.55% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AnonymousTypeDescriptor:AssertIsGood():this
         -11 (-34.38% of base) : Microsoft.CodeAnalysis.CSharp.dasm - SynthesizedGlobalMethodSymbol:SetParameters(ImmutableArray`1):this
         -11 (-32.35% of base) : Microsoft.CodeAnalysis.CSharp.dasm - EntryPointsWalker:Analyze(byref):this
         -11 (-32.35% of base) : Microsoft.CodeAnalysis.CSharp.dasm - ExitPointsWalker:Analyze(byref):this
         -83 (-28.82% of base) : System.Data.Common.dasm - RBTree`1:RemoveAt(int):this (7 methods)
         -11 (-24.44% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - PEModuleSymbol:LoadCustomAttributes(EntityHandle,byref):this
         -12 (-22.22% of base) : Microsoft.CodeAnalysis.CSharp.dasm - RetargetingSymbolTranslator:GetRetargetedAttributes(ImmutableArray`1,byref):ImmutableArray`1:this
         -11 (-21.15% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SourceNamespaceSymbol:GenerateDeclarationErrorsInTree(SyntaxTree,Nullable`1,CancellationToken):this
         -11 (-20.75% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Symbol:ForceCompleteObsoleteAttribute():this
         -11 (-20.37% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SourceNamespaceSymbol:GenerateDeclarationErrors(CancellationToken):this
         -11 (-20.37% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Symbol:ForceCompleteObsoleteAttribute():this
         -12 (-20.34% of base) : Microsoft.CodeAnalysis.CSharp.dasm - DocumentationCommentParser:ParseXmlNodes(SyntaxListBuilder`1):this
         -12 (-20.00% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpControlFlowAnalysis:get_Succeeded():bool:this
         -12 (-20.00% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicControlFlowAnalysis:get_Succeeded():bool:this
         -12 (-19.35% of base) : Microsoft.CodeAnalysis.CSharp.dasm - PEMethodSymbol:get_IsExplicitFinalizerOverride():bool:this
         -12 (-19.35% of base) : Microsoft.CodeAnalysis.CSharp.dasm - PEMethodSymbol:get_IsExplicitClassOverride():bool:this
         -12 (-19.35% of base) : Microsoft.CodeAnalysis.dasm - CryptographicHashProvider:GetHash(byref,HashAlgorithm):ImmutableArray`1:this
         -12 (-19.05% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpDataFlowAnalysis:get_Succeeded():bool:this
         -12 (-19.05% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicDataFlowAnalysis:get_Succeeded():bool:this
243 total methods with Code Size differences (243 improved, 0 regressed), 245160 unchanged.

@erozenfeld
Copy link
Member Author

erozenfeld commented Feb 3, 2020

Typical diff (ADP:UnsafeCreateTimer from System.Data.SqlClient.dll):

G_M54680_IG01:
       push     rbp
       push     r15
       push     r14
       push     rdi
       push     rsi
       push     rbx
       sub      rsp, 72
       lea      rbp, [rsp+70H]
-      xor      rax, rax
-      mov      qword ptr [rbp-38H], rax
       mov      qword ptr [rbp-40H], rsp
       mov      rsi, rcx
       mov      rdi, rdx
       mov      ebx, r8d
       mov      r14d, r9d
-					;; bbWeight=1    PerfScore 10.00
+					;; bbWeight=1    PerfScore 8.75
G_M54680_IG02:
       xor      eax, eax
       mov      dword ptr [rbp-2CH], eax
						;; bbWeight=1    PerfScore 1.25
G_M54680_IG03:
       call     ExecutionContext:IsFlowSuppressed():bool
       test     eax, eax
       jne      SHORT G_M54680_IG05
						;; bbWeight=1    PerfScore 2.25
G_M54680_IG04:
       call     ExecutionContext:SuppressFlow():AsyncFlowControl
-      mov      gword ptr [rbp-38H], rax
       mov      dword ptr [rbp-2CH], 1
-						;; bbWeight=0.50 PerfScore 1.50
+						;; bbWeight=0.50 PerfScore 1.00
G_M54680_IG05:
       mov      rcx, 0xD1FFAB1E
       call     CORINFO_HELP_NEWSFAST
       mov      r15, rax
       mov      dword ptr [rsp+20H], r14d
       mov      dword ptr [rsp+28H], 1
       mov      rcx, r15
       mov      rdx, rsi
       mov      r8, rdi
       mov      r9d, ebx
       call     Timer:.ctor(TimerCallback,Object,int,int,bool):this
       nop      
						;; bbWeight=1    PerfScore 5.75
G_M54680_IG06:
       cmp      dword ptr [rbp-2CH], 0
       je       SHORT G_M54680_IG08
						;; bbWeight=1    PerfScore 2.00
G_M54680_IG07:
       call     ExecutionContext:RestoreFlow()
						;; bbWeight=0.50 PerfScore 0.50
G_M54680_IG08:
       mov      rax, r15
						;; bbWeight=1    PerfScore 0.25
G_M54680_IG09:
       lea      rsp, [rbp-28H]
       pop      rbx
       pop      rsi
       pop      rdi
       pop      r14
       pop      r15
       pop      rbp
       ret      
						;; bbWeight=1    PerfScore 4.50
G_M54680_IG10:
       push     rbp
       push     r15
       push     r14
       push     rdi
       push     rsi
       push     rbx
       sub      rsp, 56
       mov      rbp, qword ptr [rcx+48]
       mov      qword ptr [rsp+30H], rbp
       lea      rbp, [rbp+70H]
						;; bbWeight=0    PerfScore 0.00
G_M54680_IG11:
       cmp      dword ptr [rbp-2CH], 0
       je       SHORT G_M54680_IG12
       call     ExecutionContext:RestoreFlow()
						;; bbWeight=0    PerfScore 0.00
G_M54680_IG12:
       nop      
						;; bbWeight=0    PerfScore 0.00
G_M54680_IG13:
       add      rsp, 56
       pop      rbx
       pop      rsi
       pop      rdi
       pop      r14
       pop      r15
       pop      rbp
       ret      
						;; bbWeight=0    PerfScore 0.00

; Total bytes of code 195, prolog size 39, PerfScore 47.50, (MethodHash=7b152a68) for method ADP:UnsafeCreateTimer(TimerCallback,Object,int,int):Timer
; Total bytes of code 185, prolog size 33, PerfScore 44.75, (MethodHash=7b152a68) for method ADP:UnsafeCreateTimer(TimerCallback,Object,int,int):Timer

@erozenfeld
Copy link
Member Author

erozenfeld commented Feb 3, 2020

@dotnet/jit-contrib @AndyAyersMS PTAL

@erozenfeld erozenfeld mentioned this pull request Feb 3, 2020
Closed
AndyAyersMS
AndyAyersMS approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM

@jkotas jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 3, 2020
@erozenfeld erozenfeld merged commit 9b2f32d into dotnet:master Feb 3, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndyAyersMS AndyAyersMS Awaiting requested review from AndyAyersMS

Assignees

No one assigned

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erozenfeld @AndyAyersMS @jkotas