JIT: GTF_GLOB_REF set unnecessarily on some local field accesses

[AndyAyersMS]

In particular, for address of a field.

We also should clarify or remove the x64/arm64 conservatism around implicit byref struct params.

See notes over in #845.

category:implementation
theme:ir
skill-level:expert
cost:medium

[AndyAyersMS]

Prototype: NoGlobalFlagForFieldsOfLocals

Doesn't change things much...

;; x86 diffs

Total bytes of diff: -25 (-0.00% of base)
    diff is an improvement.

Top file improvements (bytes):
         -13 : System.Net.Ping.dasm (-0.10% of base)
          -6 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
          -6 : System.Security.Cryptography.X509Certificates.dasm (-0.00% of base)

3 total files with Code Size differences (3 improved, 0 regressed), 126 unchanged.

Top method improvements (bytes):
         -13 (-5.46% of base) : System.Net.Ping.dasm - Ping:CreatePingReplyFromIcmp6EchoReply(Icmp6EchoReply,int,int):PingReply
          -6 (-0.31% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Symbol:ValidateAttributeUsage(VisualBasicAttributeData,AttributeSyntax,VisualBasicCompilation,int,DiagnosticBag,HashSet`1):bool:this
          -6 (-0.87% of base) : System.Security.Cryptography.X509Certificates.dasm - ChainPal:BuildChain(bool,ICertificatePal,X509Certificate2Collection,OidCollection,OidCollection,int,int,X509Certificate2Collection,int,DateTime,TimeSpan):ChainPal

Top method improvements (percentages):
         -13 (-5.46% of base) : System.Net.Ping.dasm - Ping:CreatePingReplyFromIcmp6EchoReply(Icmp6EchoReply,int,int):PingReply
          -6 (-0.87% of base) : System.Security.Cryptography.X509Certificates.dasm - ChainPal:BuildChain(bool,ICertificatePal,X509Certificate2Collection,OidCollection,OidCollection,int,int,X509Certificate2Collection,int,DateTime,TimeSpan):ChainPal
          -6 (-0.31% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Symbol:ValidateAttributeUsage(VisualBasicAttributeData,AttributeSyntax,VisualBasicCompilation,int,DiagnosticBag,HashSet`1):bool:this

3 total methods with Code Size differences (3 improved, 0 regressed), 208456 unchanged.

;; x64 diffs

Total bytes of diff: -327 (-0.00% of base)
    diff is an improvement.

Top file regressions (bytes):
           6 : System.Linq.Parallel.dasm (0.00% of base)

Top file improvements (bytes):
         -73 : System.Private.CoreLib.dasm (-0.00% of base)
         -38 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
         -34 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
         -24 : System.Security.Cryptography.Algorithms.dasm (-0.01% of base)
         -22 : System.Private.Xml.dasm (-0.00% of base)
         -21 : System.Security.Cryptography.Cng.dasm (-0.01% of base)
         -16 : Microsoft.CodeAnalysis.dasm (-0.00% of base)
         -12 : System.Net.Http.dasm (-0.00% of base)
         -12 : System.Runtime.Numerics.dasm (-0.02% of base)
         -12 : System.Security.Cryptography.Csp.dasm (-0.02% of base)

21 total files with Code Size differences (20 improved, 1 regressed), 108 unchanged.

Top method regressions (bytes):
           7 ( 5.07% of base) : Microsoft.CodeAnalysis.CSharp.dasm - <>c:<UnwrapAlias>b__678_0(TypeSymbol,ValueTuple`3,bool):bool:this
           6 ( 6.98% of base) : Microsoft.CodeAnalysis.CSharp.dasm - SynthesizedLambdaMethod:MakeName(String,DebugId,int,DebugId):String
           6 ( 0.67% of base) : System.Linq.Parallel.dasm - ConcatKeyComparer:Compare(ConcatKey`2,ConcatKey`2):int:this (7 methods)
           3 ( 1.02% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AnonymousTypeManager:CreatePlaceholderTemplate(AnonymousTypeKey):AnonymousTypeTemplateSymbol:this
           3 ( 1.72% of base) : System.Security.Cryptography.Cng.dasm - CngProperty:Equals(CngProperty):bool:this
           2 ( 1.07% of base) : Microsoft.CodeAnalysis.CSharp.dasm - OverloadResolution:IsApplicable(BinaryOperatorSignature,BoundExpression,BoundExpression,byref):bool:this
           2 ( 0.19% of base) : System.Security.Cryptography.Csp.dasm - CapiHelper:ToKeyBlob(RSAParameters):ref
           1 ( 0.68% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LambdaFrame:MakeName(SyntaxNode,DebugId,DebugId):String
           1 ( 0.81% of base) : System.Data.Common.dasm - SqlStringStorage:Compare(SqlString,SqlString):int:this

Top method improvements (bytes):
         -40 (-6.57% of base) : System.Private.CoreLib.dasm - ComActivator:GetClassFactoryForType(ComActivationContext):Object
         -24 (-2.83% of base) : System.Security.Cryptography.Algorithms.dasm - RSACng:ImportParameters(RSAParameters):this
         -24 (-2.83% of base) : System.Security.Cryptography.Cng.dasm - RSACng:ImportParameters(RSAParameters):this
         -22 (-4.37% of base) : System.Private.Xml.dasm - BigNumber:op_Explicit(BigNumber):double
         -16 (-4.98% of base) : System.Private.CoreLib.dasm - TupleExtensions:ToTuple(ValueTuple`1):Tuple`1 (7 methods)
         -15 (-1.78% of base) : Microsoft.CodeAnalysis.CSharp.dasm - OverloadResolution:InferMethodTypeArguments(MethodSymbol,ImmutableArray`1,AnalyzedArguments,EffectiveParameters,byref,byref):ImmutableArray`1:this
         -14 (-0.97% of base) : System.Security.Cryptography.Csp.dasm - CapiHelper:ToKeyBlob(DSAParameters):ref
         -12 (-5.00% of base) : System.Runtime.Numerics.dasm - BigInteger:op_Explicit(BigInteger):Decimal
         -11 (-9.73% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LookupResult:SetFrom(SingleLookupResult):this
          -9 (-0.27% of base) : Microsoft.CodeAnalysis.dasm - ErrorLogger:GetPropertiesValue(Issue):Value:this

Top method regressions (percentages):
           6 ( 6.98% of base) : Microsoft.CodeAnalysis.CSharp.dasm - SynthesizedLambdaMethod:MakeName(String,DebugId,int,DebugId):String
           7 ( 5.07% of base) : Microsoft.CodeAnalysis.CSharp.dasm - <>c:<UnwrapAlias>b__678_0(TypeSymbol,ValueTuple`3,bool):bool:this
           3 ( 1.72% of base) : System.Security.Cryptography.Cng.dasm - CngProperty:Equals(CngProperty):bool:this
           2 ( 1.07% of base) : Microsoft.CodeAnalysis.CSharp.dasm - OverloadResolution:IsApplicable(BinaryOperatorSignature,BoundExpression,BoundExpression,byref):bool:this
           3 ( 1.02% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AnonymousTypeManager:CreatePlaceholderTemplate(AnonymousTypeKey):AnonymousTypeTemplateSymbol:this
           1 ( 0.81% of base) : System.Data.Common.dasm - SqlStringStorage:Compare(SqlString,SqlString):int:this
           1 ( 0.68% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LambdaFrame:MakeName(SyntaxNode,DebugId,DebugId):String
           6 ( 0.67% of base) : System.Linq.Parallel.dasm - ConcatKeyComparer:Compare(ConcatKey`2,ConcatKey`2):int:this (7 methods)
           2 ( 0.19% of base) : System.Security.Cryptography.Csp.dasm - CapiHelper:ToKeyBlob(RSAParameters):ref

Top method improvements (percentages):
          -2 (-25.00% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - <>c:<.cctor>b__186_1(int,EventsToCodeAddressIndex):int:this
          -2 (-18.18% of base) : System.Private.CoreLib.dasm - Char8:CompareTo(Char8):int:this
          -3 (-16.67% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - <>c__DisplayClass14_0:<GetValue>b__0(CtfNamedRange):bool:this
          -2 (-14.29% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - <>c:<.cctor>b__186_0(int,EventsToStackIndex):int:this
          -3 (-11.11% of base) : Microsoft.CodeAnalysis.CSharp.dasm - <>c:<Analyze>b__4_0(VariableIdentifier):bool:this
         -11 (-9.73% of base) : Microsoft.CodeAnalysis.CSharp.dasm - LookupResult:SetFrom(SingleLookupResult):this
          -8 (-8.25% of base) : System.Diagnostics.Process.dasm - <>c:<GetPerformanceCounterLib>b__14_1(ValueTuple`2):PerformanceCounterLib:this
          -6 (-8.00% of base) : System.Text.RegularExpressions.dasm - SingleRangeComparer:Compare(SingleRange,SingleRange):int:this (3 methods)
          -2 (-6.67% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - _Closure$__:_Lambda$__16-9(TriviaKey,SyntaxTrivia):bool:this
         -40 (-6.57% of base) : System.Private.CoreLib.dasm - ComActivator:GetClassFactoryForType(ComActivationContext):Object

59 total methods with Code Size differences (50 improved, 9 regressed), 208874 unchanged.

[mikedn]

Prototype: NoGlobalFlagForFieldsOfLocals

FWIW I suspect that my current crusade against GT_ADDR will take care of this eventually. LocalAddressVisitor will handle more and more cases of "indirect access to a local" or "address of a local" and in the process clear GTF_GLOB_REF so at least we'll reach global morph without such issues. Remains to be seen when I'll be able to take care of the part before LocalAddressVisitor...

Also, if a variable is address exposed, morph will add back GTF_GLOB_REF and in doing so it will again "poison" all trees containing ADDR(LCL_VAR). The only safe way to avoid GTF_GLOB_REF in such cases is to use LCL_VAR_ADDR.

[AndyAyersMS]

Would be nice to get this right early and have one utility we can call. Playing around with making IsLocalAddrExpr be that utility, deprecating impIsAddressInLocal and updating fgIsIndrofAddrOfLocal.

Need to look around and see if there are any other ad-hoc classifiers.

[mikedn]

Playing around with making IsLocalAddrExpr be that utility, deprecating impIsAddressInLocal and updating fgIsIndrofAddrOfLocal.

The proper way to do this is to get rid of all that stuff, not updating it.

[AndyAyersMS]

@mikedn how do you see the importer handling these cases?

[mikedn]

how do you see the importer handling these cases?

• Import ldloca and ldarga as LCL_VAR_ADDR
• Fold ldflda and ldfld of LCL_VAR_ADDR on the fly to produce LCL_FLD_ADDR and LCL_FLD respectively. Local field access doesn't suffer from the complexities associated with non-local field access (null checks, side effects, fgUnwrapProxy etc.) so there's very little reason to defer this to global morph. This kind of deferral might be the elegant thing to do in a normal compiler but for a JIT is overkill both throughput and complexity wise.
• Maybe fold other trees on the fly, such as ldind(LCL_VAR_ADDR) or add(LCL_VAR_ADDR, 4). Not clear how common these are to worth the extra complexity.

[AndyAyersMS]

@SingleAccretion perhaps you've already addressed this one?

[SingleAccretion]

No, not yet at least.

It should get addressed with the refactoring of FIELD into FIELD_ADDR that I am yet to start working on.

[AndyAyersMS]

Still relevant.