Changes for basic block flags. by erozenfeld · Pull Request #37335 · dotnet/runtime

erozenfeld · 2020-06-03T02:31:25Z

Fix the code that propagates flags from the basic block of the return
expression to the caller's basic block during inlining. We are now
properly tracking the basic block of the return expression.
Fixes Assertion failed 'false' during 'Early Value Propagation' #36588.
Don't mark BBJ_RETURN blocks with BBF_BACKWARD_JUMP since they are
executed at most once.

The first change had a few diffs because we propagated BBF_BACKWARD_JUMP
flag. After analyzing them I realized that we shouldn't mark BBJ_RETURN
blocks with BBF_BACKWARD_JUMP in the first place. That resulted in some
diffs because we are less aggressive with inlining of calls outside of
loops.

erozenfeld · 2020-06-03T02:32:48Z

x64 framework 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: -9167 (-0.02% of base)
    diff is an improvement.
Top file regressions (bytes):
          26 : System.Private.CoreLib.dasm (0.00% of base)
           1 : System.Text.Encoding.CodePages.dasm (0.00% of base)
Top file improvements (bytes):
       -2712 : Microsoft.CodeAnalysis.CSharp.dasm (-0.06% of base)
       -1957 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.03% of base)
       -1163 : System.Private.Xml.dasm (-0.03% of base)
        -952 : System.Threading.Channels.dasm (-0.53% of base)
        -902 : Microsoft.CodeAnalysis.dasm (-0.05% of base)
        -351 : System.Reflection.Metadata.dasm (-0.08% of base)
        -193 : Newtonsoft.Json.dasm (-0.02% of base)
        -177 : System.Reflection.MetadataLoadContext.dasm (-0.10% of base)
        -131 : xunit.runner.utility.netcoreapp10.dasm (-0.07% of base)
        -130 : System.Text.Json.dasm (-0.02% of base)
        -116 : ILCompiler.Reflection.ReadyToRun.dasm (-0.09% of base)
        -101 : System.Collections.Immutable.dasm (-0.01% of base)
         -63 : System.Security.Cryptography.Cng.dasm (-0.03% of base)
         -59 : System.Drawing.Common.dasm (-0.02% of base)
         -54 : System.Linq.Expressions.dasm (-0.01% of base)
         -53 : System.DirectoryServices.AccountManagement.dasm (-0.01% of base)
         -37 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base)
         -24 : System.Data.Common.dasm (-0.00% of base)
         -12 : Microsoft.Extensions.Caching.Memory.dasm (-0.08% of base)
          -5 : Microsoft.CSharp.dasm (-0.00% of base)
23 total files with Code Size differences (21 improved, 2 regressed), 240 unchanged.
Top method regressions (bytes):
          79 ( 6.76% of base) : System.Private.CoreLib.dasm - ManifestBuilder:TranslateToManifestConvention(String,String):String:this
          21 ( 3.39% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Parser:ParseXmlMarkupDecl(SyntaxListBuilder`1):this
          13 ( 1.52% of base) : Newtonsoft.Json.dasm - JPath:TryParseValue(byref):bool:this
           9 ( 1.53% of base) : System.Private.Xml.dasm - TailCallAnalyzer:AnalyzeDefinition(QilNode)
           5 ( 0.52% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Parser:ParseNamedArguments(SeparatedSyntaxListBuilder`1):this
           1 ( 0.04% of base) : System.Text.Encoding.CodePages.dasm - GB18030Encoding:GetChars(long,int,long,int,DecoderNLS):int:this
Top method improvements (bytes):
        -728 (-8.36% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SourceMemberContainerTypeSymbol:GenerateVarianceDiagnosticsForTypeRecursively(TypeSymbol,short,int,VarianceDiagnosticsTargetTypeParameter,int,byref):this
        -358 (-40.09% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:CheckParameterNameNotDuplicate(ArrayBuilder`1,int,ParameterSyntax,ParameterSymbol,DiagnosticBag):this
        -344 (-19.95% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:ReportMutableStructureConstraintsInUsing(TypeSymbol,String,VisualBasicSyntaxNode,DiagnosticBag):this
        -282 (-46.53% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Binder:BindQueryInternal2(QueryTranslationState,DiagnosticBag):BoundExpression:this
        -243 (-20.82% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - DocumentationCommentCrefBinder:LookupSimpleNameInContainingSymbol(Symbol,bool,String,int,bool,LookupResult,int,byref):this
        -236 (-21.53% of base) : Microsoft.CodeAnalysis.dasm - SuppressMessageAttributeState:IsDiagnosticSuppressed(String,Location,byref):bool:this
        -221 (-4.74% of base) : Microsoft.CodeAnalysis.dasm - MetadataWriter:SerializeTypeReference(ITypeReference,BlobBuilder,bool,bool):this
        -213 (-16.40% of base) : Microsoft.CodeAnalysis.CSharp.dasm - ExplicitInterfaceHelpers:FindExplicitImplementationCollisions(Symbol,Symbol,DiagnosticBag)
        -187 (-15.33% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - ClsComplianceChecker:VisitMethod(MethodSymbol):this
        -182 (-11.42% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpSyntaxNode:FindTriviaByOffset(SyntaxNode,int,Func`2):SyntaxTrivia
        -177 (-23.79% of base) : System.Reflection.MetadataLoadContext.dasm - EcmaModule:ComputeEntryPoint(bool):MethodInfo:this
        -163 (-45.40% of base) : System.Reflection.Metadata.dasm - <GetTypesWithProperties>d__12:MoveNext():bool:this
        -163 (-45.40% of base) : System.Reflection.Metadata.dasm - <GetTypesWithEvents>d__13:MoveNext():bool:this
        -136 (-25.47% of base) : Microsoft.CodeAnalysis.dasm - <GetEffectiveDiagnostics>d__66:MoveNext():bool:this
        -136 (-12.08% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(__Canon):bool:this (2 methods)
        -136 (-12.22% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(ubyte):bool:this (2 methods)
        -136 (-12.21% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(short):bool:this (2 methods)
        -136 (-12.32% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(int):bool:this (2 methods)
        -136 (-12.22% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(double):bool:this (2 methods)
        -136 (-8.01% of base) : System.Threading.Channels.dasm - UnboundedChannelWriter:TryWrite(Vector`1):bool:this (2 methods)
Top method regressions (percentages):
          79 ( 6.76% of base) : System.Private.CoreLib.dasm - ManifestBuilder:TranslateToManifestConvention(String,String):String:this
          21 ( 3.39% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Parser:ParseXmlMarkupDecl(SyntaxListBuilder`1):this
           9 ( 1.53% of base) : System.Private.Xml.dasm - TailCallAnalyzer:AnalyzeDefinition(QilNode)
          13 ( 1.52% of base) : Newtonsoft.Json.dasm - JPath:TryParseValue(byref):bool:this
           5 ( 0.52% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Parser:ParseNamedArguments(SeparatedSyntaxListBuilder`1):this
           1 ( 0.04% of base) : System.Text.Encoding.CodePages.dasm - GB18030Encoding:GetChars(long,int,long,int,DecoderNLS):int:this
Top method improvements (percentages):
        -116 (-52.49% of base) : ILCompiler.Reflection.ReadyToRun.dasm - AllEntriesEnumerator:GetNext():NativeParser:this
        -282 (-46.53% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Binder:BindQueryInternal2(QueryTranslationState,DiagnosticBag):BoundExpression:this
        -163 (-45.40% of base) : System.Reflection.Metadata.dasm - <GetTypesWithProperties>d__12:MoveNext():bool:this
        -163 (-45.40% of base) : System.Reflection.Metadata.dasm - <GetTypesWithEvents>d__13:MoveNext():bool:this
         -59 (-44.70% of base) : System.Drawing.Common.dasm - Graphics:PopContext(int):this
        -358 (-40.09% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:CheckParameterNameNotDuplicate(ArrayBuilder`1,int,ParameterSyntax,ParameterSymbol,DiagnosticBag):this
         -63 (-34.62% of base) : System.Security.Cryptography.Cng.dasm - CngKey:Delete():this
         -46 (-30.87% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Scanner:EatThroughLine():this
        -109 (-26.33% of base) : Microsoft.CodeAnalysis.CSharp.dasm - DiagnosticsPass:CheckVacuousComparisons(BoundBinaryOperator,ConstantValue,BoundNode):this
        -136 (-25.47% of base) : Microsoft.CodeAnalysis.dasm - <GetEffectiveDiagnostics>d__66:MoveNext():bool:this
        -127 (-25.05% of base) : Microsoft.CodeAnalysis.dasm - SyntaxNodeOrToken:GetNextSiblingWithSearch(ChildSyntaxList):SyntaxNodeOrToken:this
        -177 (-23.79% of base) : System.Reflection.MetadataLoadContext.dasm - EcmaModule:ComputeEntryPoint(bool):MethodInfo:this
         -37 (-22.56% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - <GetEnumerable>d__15:MoveNext():bool:this
        -134 (-22.04% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Lexer:ScanXmlAttributeText(byref):this
        -236 (-21.53% of base) : Microsoft.CodeAnalysis.dasm - SuppressMessageAttributeState:IsDiagnosticSuppressed(String,Location,byref):bool:this
        -243 (-20.82% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - DocumentationCommentCrefBinder:LookupSimpleNameInContainingSymbol(Symbol,bool,String,int,bool,LookupResult,int,byref):this
        -102 (-20.52% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Lexer:ScanXmlProcessingInstructionText(byref):this
        -102 (-20.52% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Lexer:ScanXmlCommentText(byref):this
         -73 (-20.51% of base) : Microsoft.CodeAnalysis.dasm - SwitchIntegralJumpTableEmitter:EmitSwitchBuckets(ImmutableArray`1,int,int):this
         -82 (-20.40% of base) : System.Private.Xml.dasm - XmlSubtreeReader:Read():bool:this
106 total methods with Code Size differences (100 improved, 6 regressed), 243053 unchanged.

erozenfeld · 2020-06-03T02:33:09Z

@dotnet/jit-contrib @AndyAyersMS PTAL

CarolEidt · 2020-06-03T17:44:09Z

src/coreclr/src/jit/flowgraph.cpp

will pInlineInfo->retBB always be non-null if bottomBlock != nullptr?

pInlineInfo->retBB is non-null if pInlineInfo->retExpr is non null and we have a noway_assert(pInlineInfo->retExpr)` above.

Fix the code that propagates flags from the basic block of the return expression to the caller's basic block during inlining. We are now properly tracking the basic block of the return expression. Fixes dotnet#36588. Don't mark BBJ_RETURN blocks with BBF_BACKWARD_JUMP since they are executed at most once. The first change had a few diffs because we propagated BBF_BACKWARD_JUMP flag. After analyzing them I realized that we shouldn't mark BBJ_RETURN blocks with BBF_BACKWARD_JUMP in the first place. That resulted in some diffs because we are less aggressive with inlining of calls outside of loops.

erozenfeld · 2020-06-04T21:06:12Z

I decided to change my fix to update the basic block flags later in fgUpdateInlineReturnExpressionPlaceHolder when we replace GT_RET_EXPR with the tree from the inlinee. I think it's a cleaner fix even though it requires more plumbing. I also added a comment.
The diffs are the same as with the previous version of the fix.
@CarolEidt @AndyAyersMS PTAL

BruceForstall

LGTM

AndyAyersMS

LGTM.

This is a fllow-up to dotnet#37335 and dotnet#37840. When an inline fails we replace `GT_RET_EXPR`with the original `GT_CALL` node. `GT_RET_EXPR`may end up in a basic block other than the original `GT_CALL` so we need to propagate basic block flags. Fixes dotnet#36588.

This is a fllow-up to #37335 and #37840. When an inline fails we replace `GT_RET_EXPR`with the original `GT_CALL` node. `GT_RET_EXPR`may end up in a basic block other than the original `GT_CALL` so we need to propagate basic block flags. Fixes #36588.

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

erozenfeld requested a review from AndyAyersMS June 3, 2020 02:33

CarolEidt reviewed Jun 3, 2020

View reviewed changes

erozenfeld force-pushed the BBFlags branch from 3e75b3a to c1f9e25 Compare June 4, 2020 21:02

BruceForstall approved these changes Jun 4, 2020

View reviewed changes

AndyAyersMS approved these changes Jun 4, 2020

View reviewed changes

erozenfeld merged commit f326f52 into dotnet:master Jun 5, 2020

sandreenko mentioned this pull request Jun 8, 2020

Test failure: JIT/Performance/CodeQuality/BenchmarksGame/reverse-complement/reverse-complement-1/reverse-complement-1.sh #37574

Closed

erozenfeld mentioned this pull request Jun 13, 2020

Fix propagation of basic block flags for inlinee return expressions. #37840

Merged

erozenfeld mentioned this pull request Jun 22, 2020

Propagate basic block flags after a failed inline. #38243

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for basic block flags.#37335

Changes for basic block flags.#37335
erozenfeld merged 1 commit intodotnet:masterfrom
erozenfeld:BBFlags

erozenfeld commented Jun 3, 2020 •

edited

Loading

Uh oh!

erozenfeld commented Jun 3, 2020

Uh oh!

erozenfeld commented Jun 3, 2020

Uh oh!

CarolEidt Jun 3, 2020

Uh oh!

erozenfeld Jun 4, 2020

Uh oh!

erozenfeld commented Jun 4, 2020

Uh oh!

BruceForstall left a comment

Uh oh!

AndyAyersMS left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

erozenfeld commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erozenfeld commented Jun 3, 2020

Uh oh!

erozenfeld commented Jun 3, 2020

Uh oh!

CarolEidt Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

erozenfeld Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

erozenfeld commented Jun 4, 2020

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erozenfeld commented Jun 3, 2020 •

edited

Loading