Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes for basic block flags. #37335

Merged
merged 1 commit into from
Jun 5, 2020
Merged

Conversation

erozenfeld
Copy link
Member

@erozenfeld erozenfeld commented Jun 3, 2020

  1. 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.

  2. 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.

@Dotnet-GitSync-Bot 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
Copy link
Member Author

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
Copy link
Member Author

@dotnet/jit-contrib @AndyAyersMS PTAL

@@ -23540,7 +23541,7 @@ void Compiler::fgInsertInlineeBlocks(InlineInfo* pInlineInfo)

if (bottomBlock != nullptr)
{
bottomBlock->bbFlags |= InlineeCompiler->fgLastBB->bbFlags & BBF_SPLIT_GAINED;
bottomBlock->bbFlags |= pInlineInfo->retBB->bbFlags & BBF_SPLIT_GAINED;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

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.

@erozenfeld erozenfeld merged commit f326f52 into dotnet:master Jun 5, 2020
erozenfeld added a commit to erozenfeld/runtime that referenced this pull request Jun 22, 2020
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.
erozenfeld added a commit that referenced this pull request Jun 23, 2020
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.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failed 'false' during 'Early Value Propagation'
5 participants