Fix `fgUpdateChangedFlowGraph`. #39878

erozenfeld · 2020-07-24T01:02:22Z

fgComputeDoms has an assumption that the flow graph
has no unreachable blocks. It's checked with this assert:

runtime/src/coreclr/src/jit/flowgraph.cpp

Line 2342 in ad325b0

noway_assert(postIndex == fgBBcount + 1);

This assert fired when testing #39474 (Convert Math{F}.CoreCLR methods from FCall to QCall)
when we are updating the flow graph after inserting GC polls.

This change switches fgUpdateChangedFlowGraph to call fgComputeReachability,
which both removes unreachable blocks and calls fgComputeDoms.

pin-icount reported a 0.0043% throughput improvement, which is within noise level.

`fgComputeDoms` has an assumption that the flow graph has no unreachable blocks. It's checked with this assert: https://github.com/dotnet/runtime/blob/ad325b014124b1adb9306abf95fdac85e3f7f2f4/src/coreclr/src/jit/flowgraph.cpp#L2342 This assert fired when testing dotnet#39474 (`Convert Math{F}.CoreCLR methods from FCall to QCall`) when we are updating the flow graph after inserting GC polls. This change switches `fgUpdateChangedFlowGraph` to call `fgComputeReachability`, which both removes unreachable blocks and calls `fgComputeDoms`. pin-icount reported a 0.0043% throughput improvement, which is within noise level.

erozenfeld · 2020-07-24T01:03:39Z

Fixes one of the asserts seen in #39474 and partially resolves #39726.

erozenfeld · 2020-07-24T01:04:56Z

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: -374 (-0.001% of base)
    diff is an improvement.
Top file regressions (bytes):
         274 : System.Private.Xml.dasm (0.008% of base)
         128 : System.Text.RegularExpressions.dasm (0.052% of base)
          88 : System.Collections.Concurrent.dasm (0.026% of base)
          84 : System.Threading.Channels.dasm (0.049% of base)
          65 : System.Private.CoreLib.dasm (0.001% of base)
          64 : Microsoft.CodeAnalysis.CSharp.dasm (0.001% of base)
          16 : System.Runtime.Numerics.dasm (0.022% of base)
          12 : Microsoft.Diagnostics.FastSerialization.dasm (0.012% of base)
           9 : System.Linq.dasm (0.001% of base)
           5 : ILCompiler.Reflection.ReadyToRun.dasm (0.004% of base)
           3 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.000% of base)
           3 : System.DirectoryServices.AccountManagement.dasm (0.001% of base)
           2 : Microsoft.VisualBasic.Core.dasm (0.000% of base)
           2 : Newtonsoft.Json.dasm (0.000% of base)
           2 : System.Net.Http.dasm (0.000% of base)
Top file improvements (bytes):
       -1001 : System.Text.Json.dasm (-0.179% of base)
         -60 : System.Private.Uri.dasm (-0.070% of base)
         -19 : Microsoft.CSharp.dasm (-0.005% of base)
         -15 : Microsoft.CodeAnalysis.dasm (-0.001% of base)
         -15 : System.Data.Common.dasm (-0.001% of base)
         -10 : System.Text.Encodings.Web.dasm (-0.030% of base)
          -6 : System.Net.Primitives.dasm (-0.009% of base)
          -3 : System.Net.Mail.dasm (-0.002% of base)
          -2 : System.IO.Compression.dasm (-0.003% of base)
24 total files with Code Size differences (9 improved, 15 regressed), 241 unchanged.
Top method regressions (bytes):
          70 (0.373% of base) : System.Text.Json.dasm - ObjectWithParameterizedConstructorConverter`1:OnTryRead(byref,Type,JsonSerializerOptions,byref,byref):bool:this (7 methods)
          66 (9.579% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(double):bool:this (2 methods)
          64 (1.341% of base) : System.Text.RegularExpressions.dasm - RegexRunner:Scan(Regex,String,int,byref,MatchCallback`1,TimeSpan):this (8 methods)
          60 (2.169% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseNamespaceBody(byref,byref,byref,ushort):this
          36 (4.675% of base) : System.Text.RegularExpressions.dasm - RegexBoyerMoore:Scan(String,int,int,int):int:this
          34 (0.695% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:PickPivotAndPartition(Span`1,Span`1,IComparer`1):int (7 methods)
          33 (1.260% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:IncrementalRead():int:this
          32 (1.031% of base) : Newtonsoft.Json.dasm - JsonTextReader:ReadStringIntoBuffer(ushort):this
          27 (1.761% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:ParseAttributes():this
          24 (2.154% of base) : System.Private.CoreLib.dasm - ManifestBuilder:TranslateToManifestConvention(String,String):String:this
          23 (2.987% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - MemberLookup:LookupInClass(LookupResult,TypeSymbol,String,int,int,TypeSymbol,Binder,byref)
          22 (0.784% of base) : System.Text.RegularExpressions.dasm - RegexWriter:RegexCodeFromRegexTree(RegexTree):RegexCode:this (2 methods)
          17 (3.656% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,__Canon):long:this
          17 (3.680% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,ubyte):long:this
          17 (3.680% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,short):long:this
          17 (3.704% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,int):long:this
          17 (3.696% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,long):long:this
          16 (6.987% of base) : System.Private.CoreLib.dasm - Number:FindSection(ReadOnlySpan`1,int):int
          16 (3.893% of base) : System.Private.CoreLib.dasm - UmAlQuraCalendar:ConvertGregorianToHijri(DateTime,byref,byref,byref)
          16 (6.987% of base) : System.Runtime.Numerics.dasm - Number:FindSection(ReadOnlySpan`1,int):int
Top method improvements (bytes):
        -665 (-4.507% of base) : System.Text.Json.dasm - ObjectWithParameterizedConstructorConverter`1:TryLookupConstructorParameter(byref,byref,JsonSerializerOptions,byref):bool:this (7 methods)
        -193 (-7.985% of base) : System.Text.Json.dasm - JsonSerializer:LookupProperty(Object,ReadOnlySpan`1,byref,byref,bool):JsonPropertyInfo
        -108 (-5.947% of base) : System.Text.Json.dasm - JsonClassInfo:GetProperty(ReadOnlySpan`1,byref,byref):JsonPropertyInfo:this
        -105 (-5.863% of base) : System.Text.Json.dasm - JsonClassInfo:GetParameter(ReadOnlySpan`1,byref,byref):JsonParameterInfo:this
         -60 (-9.434% of base) : System.Private.Uri.dasm - Uri:UnescapeOnly(long,int,byref,ushort,ushort,ushort)
         -49 (-3.030% of base) : System.Private.CoreLib.dasm - ReaderWriterLockSlim:TryEnterWriteLockCore(TimeoutTracker):bool:this
         -29 (-2.805% of base) : Newtonsoft.Json.dasm - JsonTextReader:ParseComment(bool):this
         -16 (-0.647% of base) : Microsoft.CSharp.dasm - ExpressionBinder:bindUDUnop(int,Expr):Expr:this
         -14 (-1.320% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SyntaxNodeExtensions:ExtractAnonymousTypeMemberName(ExpressionSyntax,byref):SyntaxToken
         -13 (-1.045% of base) : Microsoft.CodeAnalysis.dasm - Parser:GetMatchingMethods(String,byref,List`1,String,int,Compilation,List`1)
         -13 (-0.710% of base) : System.Data.Common.dasm - XmlDataLoader:LoadTopMostTable(DataTable):this
         -11 (-1.259% of base) : Newtonsoft.Json.dasm - JsonTextReader:ParseConstructor():this
         -10 (-0.645% of base) : System.Text.Encodings.Web.dasm - TextEncoder:EncodeUtf8(ReadOnlySpan`1,Span`1,byref,byref,bool):int:this
          -7 (-0.864% of base) : Microsoft.CodeAnalysis.CSharp.dasm - InterpolatedStringScanner:ScanFormatSpecifier():this
          -6 (-0.493% of base) : System.Net.Primitives.dasm - CookieContainer:CookieCutter(Uri,String,String,bool):CookieCollection:this
          -5 (-1.326% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseVariableDeclarators(TypeSyntax,int,SeparatedSyntaxListBuilder`1,bool):this
          -5 (-0.238% of base) : Microsoft.CodeAnalysis.dasm - TargetSymbolResolver:Resolve(IList`1):this
          -5 (-0.135% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Parser:ParseXmlDeclaration():XmlDeclarationSyntax:this
          -5 (-0.960% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,Vector`1):long:this
          -4 (-0.176% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Parser:ParseXmlElement(int):XmlNodeSyntax:this
Top method regressions (percentages):
          66 (9.579% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(double):bool:this (2 methods)
          16 (6.987% of base) : System.Private.CoreLib.dasm - Number:FindSection(ReadOnlySpan`1,int):int
          16 (6.987% of base) : System.Runtime.Numerics.dasm - Number:FindSection(ReadOnlySpan`1,int):int
          10 (4.878% of base) : Newtonsoft.Json.dasm - JsonTextReader:ParseUnquotedProperty():this
          36 (4.675% of base) : System.Text.RegularExpressions.dasm - RegexBoyerMoore:Scan(String,int,int,int):int:this
          16 (3.893% of base) : System.Private.CoreLib.dasm - UmAlQuraCalendar:ConvertGregorianToHijri(DateTime,byref,byref,byref)
          17 (3.704% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,int):long:this
          17 (3.696% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,long):long:this
          17 (3.680% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,ubyte):long:this
          17 (3.680% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,short):long:this
          17 (3.656% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,__Canon):long:this
          14 (3.211% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`2,Func`3):long:this
           7 (3.139% of base) : System.Private.CoreLib.dasm - HashtableEnumerator:MoveNext():bool:this
           6 (3.030% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AccessCheck:InheritsFromIgnoringConstruction(TypeSymbol,NamedTypeSymbol,CSharpCompilation,byref,ConsList`1):bool
          23 (2.987% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - MemberLookup:LookupInClass(LookupResult,TypeSymbol,String,int,int,TypeSymbol,Binder,byref)
          14 (2.954% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,double):long:this
           6 (2.542% of base) : System.Private.Xml.dasm - XPathNodeHelper:GetPreviousContentSibling(byref,byref):bool
          60 (2.169% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseNamespaceBody(byref,byref,byref,ushort):this
          24 (2.154% of base) : System.Private.CoreLib.dasm - ManifestBuilder:TranslateToManifestConvention(String,String):String:this
          11 (2.136% of base) : System.Private.Xml.dasm - XmlEncodedRawTextWriter:RawTextNoFlush(long,long):int:this
Top method improvements (percentages):
         -60 (-9.434% of base) : System.Private.Uri.dasm - Uri:UnescapeOnly(long,int,byref,ushort,ushort,ushort)
        -193 (-7.985% of base) : System.Text.Json.dasm - JsonSerializer:LookupProperty(Object,ReadOnlySpan`1,byref,byref,bool):JsonPropertyInfo
        -108 (-5.947% of base) : System.Text.Json.dasm - JsonClassInfo:GetProperty(ReadOnlySpan`1,byref,byref):JsonPropertyInfo:this
        -105 (-5.863% of base) : System.Text.Json.dasm - JsonClassInfo:GetParameter(ReadOnlySpan`1,byref,byref):JsonParameterInfo:this
        -665 (-4.507% of base) : System.Text.Json.dasm - ObjectWithParameterizedConstructorConverter`1:TryLookupConstructorParameter(byref,byref,JsonSerializerOptions,byref):bool:this (7 methods)
         -49 (-3.030% of base) : System.Private.CoreLib.dasm - ReaderWriterLockSlim:TryEnterWriteLockCore(TimeoutTracker):bool:this
         -29 (-2.805% of base) : Newtonsoft.Json.dasm - JsonTextReader:ParseComment(bool):this
          -5 (-1.326% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LanguageParser:ParseVariableDeclarators(TypeSyntax,int,SeparatedSyntaxListBuilder`1,bool):this
         -14 (-1.320% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SyntaxNodeExtensions:ExtractAnonymousTypeMemberName(ExpressionSyntax,byref):SyntaxToken
         -11 (-1.259% of base) : Newtonsoft.Json.dasm - JsonTextReader:ParseConstructor():this
         -13 (-1.045% of base) : Microsoft.CodeAnalysis.dasm - Parser:GetMatchingMethods(String,byref,List`1,String,int,Compilation,List`1)
          -5 (-0.960% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(double,Func`3,Func`4,Vector`1):long:this
          -7 (-0.864% of base) : Microsoft.CodeAnalysis.CSharp.dasm - InterpolatedStringScanner:ScanFormatSpecifier():this
         -13 (-0.710% of base) : System.Data.Common.dasm - XmlDataLoader:LoadTopMostTable(DataTable):this
         -16 (-0.647% of base) : Microsoft.CSharp.dasm - ExpressionBinder:bindUDUnop(int,Expr):Expr:this
         -10 (-0.645% of base) : System.Text.Encodings.Web.dasm - TextEncoder:EncodeUtf8(ReadOnlySpan`1,Span`1,byref,byref,bool):int:this
          -2 (-0.623% of base) : System.IO.Compression.dasm - DeflateManagedStream:Read(ref,int,int):int:this
          -3 (-0.620% of base) : System.Private.Xml.dasm - NumberFormatter:ConvertToDecimal(double,int,ushort,String,int):String
          -2 (-0.515% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(int,Func`3,Func`4,Vector`1):long:this
          -2 (-0.494% of base) : System.Collections.Concurrent.dasm - ConcurrentDictionary`2:AddOrUpdate(long,Func`3,Func`4,Vector`1):long:this
139 total methods with Code Size differences (46 improved, 93 regressed), 244314 unchanged.

erozenfeld · 2020-07-24T01:08:21Z

In addition to removing unreachable blocks fgRemoveUnreachableBlocks updates BBF_LOOP_HEAD flags even if no unreachable blocks were found. Some of the diffs above are caused by downstream effects from that, e.g., in some cases we now recognize more loops, which changes weights, etc.

erozenfeld · 2020-07-24T01:08:52Z

@AndyAyersMS @dotnet/jit-contrib PTAL

am11 · 2020-07-24T22:18:48Z

Build Linux x64 release SourceBuild is green in AzDo, but GitHub is not updated. (it is happening in other PRs as well)

erozenfeld · 2020-07-25T03:55:41Z

Build Linux x64 release SourceBuild is green in AzDo, but GitHub is not updated. (it is happening in other PRs as well)

CoreCLR Product Build Windows_NT arm checked is also green in AzDo but not updated in GitHub.

In dotnet#39878 I switched fgUpdateChangedFlowGraph to call fgComputeReachability, which both removes unreachable blocks and calls fgComputeDoms. As mentioned in that PR, in addition to removing unreachable blocks fgRemoveUnreachableBlocks updates `BBF_LOOP_HEAD` flags even if no unreachable blocks were found. That resulted in some diffs, both positive and negative, from downstream effects: e.g., in some cases we now recognize more loops, which changes weights, etc. Some of the negative diffs affected benchmarks we are tracking, e.g., in dotnet#40094 `System.Text.RegularExpressions.Tests.Perf_Regex_Common` had a 10% regression because of codegen diffs in the large dynamic method created when compiling regular expressions. Because of these regressions, I decided to go with a more surgical fix for the original issue (assert when computing dominators after inlining GC polls). The downstream phases don't really need the dominator info so I changed fgUpdateChangedFlowGraph to not re-compute dominators after GC poll inlining. This reverses all diffs from dotnet#39878 and fixes dotnet#40094.

In #39878 I switched fgUpdateChangedFlowGraph to call fgComputeReachability, which both removes unreachable blocks and calls fgComputeDoms. As mentioned in that PR, in addition to removing unreachable blocks fgRemoveUnreachableBlocks updates `BBF_LOOP_HEAD` flags even if no unreachable blocks were found. That resulted in some diffs, both positive and negative, from downstream effects: e.g., in some cases we now recognize more loops, which changes weights, etc. Some of the negative diffs affected benchmarks we are tracking, e.g., in #40094 `System.Text.RegularExpressions.Tests.Perf_Regex_Common` had a 10% regression because of codegen diffs in the large dynamic method created when compiling regular expressions. Because of these regressions, I decided to go with a more surgical fix for the original issue (assert when computing dominators after inlining GC polls). The downstream phases don't really need the dominator info so I changed fgUpdateChangedFlowGraph to not re-compute dominators after GC poll inlining. This reverses all diffs from #39878 and fixes #40094.

`fgComputeDoms` has an assumption that the flow graph has no unreachable blocks. It's checked with this assert: https://github.com/dotnet/runtime/blob/ad325b014124b1adb9306abf95fdac85e3f7f2f4/src/coreclr/src/jit/flowgraph.cpp#L2342 This assert fired when testing dotnet#39474 (`Convert Math{F}.CoreCLR methods from FCall to QCall`) when we are updating the flow graph after inserting GC polls. This change switches `fgUpdateChangedFlowGraph` to call `fgComputeReachability`, which both removes unreachable blocks and calls `fgComputeDoms`. pin-icount reported a 0.0043% throughput improvement, which is within noise level.

In dotnet#39878 I switched fgUpdateChangedFlowGraph to call fgComputeReachability, which both removes unreachable blocks and calls fgComputeDoms. As mentioned in that PR, in addition to removing unreachable blocks fgRemoveUnreachableBlocks updates `BBF_LOOP_HEAD` flags even if no unreachable blocks were found. That resulted in some diffs, both positive and negative, from downstream effects: e.g., in some cases we now recognize more loops, which changes weights, etc. Some of the negative diffs affected benchmarks we are tracking, e.g., in dotnet#40094 `System.Text.RegularExpressions.Tests.Perf_Regex_Common` had a 10% regression because of codegen diffs in the large dynamic method created when compiling regular expressions. Because of these regressions, I decided to go with a more surgical fix for the original issue (assert when computing dominators after inlining GC polls). The downstream phases don't really need the dominator info so I changed fgUpdateChangedFlowGraph to not re-compute dominators after GC poll inlining. This reverses all diffs from dotnet#39878 and fixes dotnet#40094.

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 24, 2020

erozenfeld requested a review from AndyAyersMS July 24, 2020 01:08

erozenfeld mentioned this pull request Jul 24, 2020

Convert Math{F}.CoreCLR methods from FCall to QCall #39474

Closed

2 tasks

AndyAyersMS approved these changes Jul 24, 2020

View reviewed changes

erozenfeld closed this Jul 24, 2020

erozenfeld reopened this Jul 24, 2020

erozenfeld merged commit b179d69 into dotnet:master Jul 25, 2020

erozenfeld mentioned this pull request Jul 25, 2020

SuppressGCTransitionAttribute hitting asserts in the JIT #39726

Closed

danmoseley mentioned this pull request Jul 29, 2020

[Perf -10%] System.Text.RegularExpressions.Tests.Perf_Regex_Common (2) #40094

Closed

erozenfeld mentioned this pull request Jul 30, 2020

Re-work fix for flow graph update. #40162

Merged

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `fgUpdateChangedFlowGraph`. #39878

Fix `fgUpdateChangedFlowGraph`. #39878

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

am11 commented Jul 24, 2020

erozenfeld commented Jul 25, 2020

Fix fgUpdateChangedFlowGraph. #39878

Fix fgUpdateChangedFlowGraph. #39878

Conversation

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

erozenfeld commented Jul 24, 2020

am11 commented Jul 24, 2020

erozenfeld commented Jul 25, 2020

Fix `fgUpdateChangedFlowGraph`. #39878

Fix `fgUpdateChangedFlowGraph`. #39878