Skip to content

Conversation

@SingleAccretion
Copy link
Contributor

See #49113 for context.

The change is two-fold: refactoring of optRemoveRangeCheck to handle both types of checks ("flattened" COMMAs and regular COMMAs) and threading the support through the relevant phases.

I decided to concentrate the new logic in optRemoveRangeCheck instead of leaning on the calling code to check what type of check do they have. This lead to a non-obvious (and arguably non-ideal) design, but that was the best I could come up with.

I also added two helpers that wrap this new optRemoveRangeCheck for code that does know what type of check it is dealing with.

This fixes the original reproduction:

public static void CheckZero(Span<int> a) { static int Value() => 42; if (!a.IsEmpty) { a[0] = Value(); } }
G_M7183_IG02:  mov rax, bword ptr [rcx]  mov edx, dword ptr [rcx+8] G_M7183_IG03:  test edx, edx  jbe SHORT G_M7183_IG05 G_M7183_IG04:  mov dword ptr [rax], 42 G_M7183_IG05:  ret

It also works for a non-zero constant (the change in range check elimination is responsible for that):

public static void CheckOne(Span<int> a) { static int Value() => 42; if (a.Length > 1) { a[0] = Value(); } }
 mov rax, bword ptr [rcx]  mov edx, dword ptr [rcx+8] G_M7183_IG03:  cmp edx, 1  jle SHORT G_M7183_IG05 G_M7183_IG04:  mov dword ptr [rax], 42 G_M7183_IG05:  ret

There are some diffs as well (mostly in CoreLib):

The diffs
Crossgen CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for default jit Summary of Code Size diffs: (Lower is better) Total bytes of base: 33745766 Total bytes of diff: 33745610 Total bytes of delta: -156 (-0.00% of base) diff is an improvement. Top file improvements (bytes): -120 : System.Private.CoreLib.dasm (-0.00% of base) -27 : System.Net.Http.dasm (-0.00% of base) -9 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base) 3 total files with Code Size differences (3 improved, 0 regressed), 268 unchanged. Top method improvements (bytes): -27 (-9.31% of base) : System.Net.Http.dasm - System.Net.Http.HPack.IntegerEncoder:Encode(int,int,System.Span`1[Byte],byref):bool -26 (-34.21% of base) : System.Private.CoreLib.dasm - System.Text.ASCIIEncoding:EncodeRune(System.Text.Rune,System.Span`1[Byte],byref):int:this -26 (-32.91% of base) : System.Private.CoreLib.dasm - System.Text.Latin1Encoding:EncodeRune(System.Text.Rune,System.Span`1[Byte],byref):int:this -25 (-28.09% of base) : System.Private.CoreLib.dasm - System.Decimal:TryGetBits(System.Decimal,System.Span`1[Int32],byref):bool -15 (-19.48% of base) : System.Private.CoreLib.dasm - System.Runtime.InteropServices.ComEventsSink:GetVariant(byref):byref -10 (-13.16% of base) : System.Private.CoreLib.dasm - System.Decimal:GetBits(System.Decimal,System.Span`1[Int32]):int -10 (-6.85% of base) : System.Private.CoreLib.dasm - System.Text.Rune:TryEncodeToUtf16(System.Span`1[Char],byref):bool:this -9 (-4.15% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Ctf.CtfMetadataLegacyParser:FindCloseBrace(System.String,int):int -8 (-1.28% of base) : System.Private.CoreLib.dasm - System.IO.Path:Populate83FileNameFromRandomBytes(long,int,System.Span`1[Char]) Top method improvements (percentages): -26 (-34.21% of base) : System.Private.CoreLib.dasm - System.Text.ASCIIEncoding:EncodeRune(System.Text.Rune,System.Span`1[Byte],byref):int:this -26 (-32.91% of base) : System.Private.CoreLib.dasm - System.Text.Latin1Encoding:EncodeRune(System.Text.Rune,System.Span`1[Byte],byref):int:this -25 (-28.09% of base) : System.Private.CoreLib.dasm - System.Decimal:TryGetBits(System.Decimal,System.Span`1[Int32],byref):bool -15 (-19.48% of base) : System.Private.CoreLib.dasm - System.Runtime.InteropServices.ComEventsSink:GetVariant(byref):byref -10 (-13.16% of base) : System.Private.CoreLib.dasm - System.Decimal:GetBits(System.Decimal,System.Span`1[Int32]):int -27 (-9.31% of base) : System.Net.Http.dasm - System.Net.Http.HPack.IntegerEncoder:Encode(int,int,System.Span`1[Byte],byref):bool -10 (-6.85% of base) : System.Private.CoreLib.dasm - System.Text.Rune:TryEncodeToUtf16(System.Span`1[Char],byref):bool:this -9 (-4.15% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.Ctf.CtfMetadataLegacyParser:FindCloseBrace(System.String,int):int -8 (-1.28% of base) : System.Private.CoreLib.dasm - System.IO.Path:Populate83FileNameFromRandomBytes(long,int,System.Span`1[Char]) -------------------------------------------------------------------------------- benchmarks.run.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 7261 Total bytes of diff: 7078 Total bytes of delta: -183 (-2.52% of base) diff is an improvement. Top file improvements (bytes): -61 : 5812.dasm (-2.37% of base) -60 : 5896.dasm (-2.28% of base) -27 : 5425.dasm (-9.31% of base) -13 : 3441.dasm (-1.34% of base) -10 : 10817.dasm (-6.90% of base) -8 : 5026.dasm (-1.39% of base) -4 : 15357.dasm (-5.48% of base) 7 total files with Code Size differences (7 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -61 (-2.37% of base) : 5812.dasm - System.Net.Security.SafeDeleteContext:InitializeSecurityContext(byref,byref,System.String,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -60 (-2.28% of base) : 5896.dasm - System.Net.Security.SafeDeleteContext:AcceptSecurityContext(byref,byref,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -27 (-9.31% of base) : 5425.dasm - System.Net.Http.HPack.IntegerEncoder:Encode(int,int,System.Span`1[Byte],byref):bool -13 (-1.34% of base) : 3441.dasm - System.Text.RegularExpressions.RegexCharClass:ToStringClass(byref):this -10 (-6.90% of base) : 10817.dasm - System.Text.Rune:TryEncodeToUtf16(System.Span`1[Char],byref):bool:this -8 (-1.39% of base) : 5026.dasm - System.IO.Path:Populate83FileNameFromRandomBytes(long,int,System.Span`1[Char]) -4 (-5.48% of base) : 15357.dasm - Span.IndexerBench:TestWriteViaIndexer1(System.Span`1[Byte]):ubyte Top method improvements (percentages): -27 (-9.31% of base) : 5425.dasm - System.Net.Http.HPack.IntegerEncoder:Encode(int,int,System.Span`1[Byte],byref):bool -10 (-6.90% of base) : 10817.dasm - System.Text.Rune:TryEncodeToUtf16(System.Span`1[Char],byref):bool:this -4 (-5.48% of base) : 15357.dasm - Span.IndexerBench:TestWriteViaIndexer1(System.Span`1[Byte]):ubyte -61 (-2.37% of base) : 5812.dasm - System.Net.Security.SafeDeleteContext:InitializeSecurityContext(byref,byref,System.String,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -60 (-2.28% of base) : 5896.dasm - System.Net.Security.SafeDeleteContext:AcceptSecurityContext(byref,byref,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -8 (-1.39% of base) : 5026.dasm - System.IO.Path:Populate83FileNameFromRandomBytes(long,int,System.Span`1[Char]) -13 (-1.34% of base) : 3441.dasm - System.Text.RegularExpressions.RegexCharClass:ToStringClass(byref):this 7 total methods with Code Size differences (7 improved, 0 regressed), 0 unchanged. -------------------------------------------------------------------------------- libraries.pmi.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 32144 Total bytes of diff: 31341 Total bytes of delta: -803 (-2.50% of base) diff is an improvement. Top file improvements (bytes): -61 : 111334.dasm (-2.37% of base) -61 : 197642.dasm (-2.37% of base) -61 : 196472.dasm (-2.37% of base) -61 : 201258.dasm (-2.37% of base) -60 : 201260.dasm (-2.28% of base) -60 : 196474.dasm (-2.28% of base) -60 : 111336.dasm (-2.28% of base) -60 : 197644.dasm (-2.28% of base) -27 : 112999.dasm (-9.31% of base) -27 : 143170.dasm (-45.00% of base) -19 : 197639.dasm (-2.88% of base) -19 : 203166.dasm (-8.84% of base) -19 : 203165.dasm (-9.36% of base) -19 : 197638.dasm (-2.19% of base) -19 : 201213.dasm (-1.99% of base) -19 : 203164.dasm (-9.18% of base) -19 : 203168.dasm (-9.36% of base) -19 : 201214.dasm (-2.22% of base) -19 : 203163.dasm (-9.18% of base) -18 : 209389.dasm (-0.90% of base) 26 total files with Code Size differences (26 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -61 (-2.37% of base) : 201258.dasm - System.Net.Security.SafeDeleteContext:InitializeSecurityContext(byref,byref,System.String,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -60 (-2.28% of base) : 201260.dasm - System.Net.Security.SafeDeleteContext:AcceptSecurityContext(byref,byref,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -27 (-9.31% of base) : 112999.dasm - System.Net.Http.HPack.IntegerEncoder:Encode(int,int,System.Span`1[Byte],byref):bool -27 (-45.00% of base) : 143170.dasm - System.Runtime.InteropServices.ComEventsSink:GetVariant(byref):byref -19 (-2.88% of base) : 197639.dasm - System.Net.Security.NegotiateStreamPal:MakeSignature(System.Net.Security.SafeDeleteContext,System.Byte[],int,int,byref):int -19 (-8.84% of base) : 203166.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,double):System.Numerics.Tensors.Tensor`1[Double] -19 (-9.36% of base) : 203165.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,int):System.Numerics.Tensors.Tensor`1[Int32] -19 (-2.19% of base) : 197638.dasm - System.Net.Security.NegotiateStreamPal:VerifySignature(System.Net.Security.SafeDeleteContext,System.Byte[],int,int):int -19 (-1.99% of base) : 201213.dasm - System.Net.Security.NegotiateStreamPal:Decrypt(System.Net.Security.SafeDeleteContext,System.Byte[],int,int,bool,bool,byref,int):int -19 (-9.18% of base) : 203164.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,short):System.Numerics.Tensors.Tensor`1[Int16] -19 (-9.36% of base) : 203168.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,long):System.Numerics.Tensors.Tensor`1[Int64] -19 (-2.22% of base) : 201214.dasm - System.Net.Security.NegotiateStreamPal:DecryptNtlm(System.Net.Security.SafeDeleteContext,System.Byte[],int,int,bool,byref,int):int -19 (-9.18% of base) : 203163.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,ubyte):System.Numerics.Tensors.Tensor`1[Byte] -18 (-0.90% of base) : 209389.dasm - Internal.Cryptography.Pbkdf2Implementation:FillKeyDerivation(System.ReadOnlySpan`1[Byte],System.ReadOnlySpan`1[Byte],int,System.String,System.Span`1[Byte]) Top method improvements (percentages): -27 (-45.00% of base) : 143170.dasm - System.Runtime.InteropServices.ComEventsSink:GetVariant(byref):byref -19 (-9.36% of base) : 203165.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,int):System.Numerics.Tensors.Tensor`1[Int32] -19 (-9.36% of base) : 203168.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,long):System.Numerics.Tensors.Tensor`1[Int64] -27 (-9.31% of base) : 112999.dasm - System.Net.Http.HPack.IntegerEncoder:Encode(int,int,System.Span`1[Byte],byref):bool -19 (-9.18% of base) : 203164.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,short):System.Numerics.Tensors.Tensor`1[Int16] -19 (-9.18% of base) : 203163.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,ubyte):System.Numerics.Tensors.Tensor`1[Byte] -19 (-8.84% of base) : 203166.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,double):System.Numerics.Tensors.Tensor`1[Double] -15 (-7.46% of base) : 203162.dasm - System.Numerics.Tensors.Tensor:CreateIdentity(int,bool,System.__Canon):System.Numerics.Tensors.Tensor`1[__Canon] -9 (-4.04% of base) : 99528.dasm - Microsoft.Diagnostics.Tracing.Ctf.CtfMetadataLegacyParser:FindCloseBrace(System.String,int):int -19 (-2.88% of base) : 197639.dasm - System.Net.Security.NegotiateStreamPal:MakeSignature(System.Net.Security.SafeDeleteContext,System.Byte[],int,int,byref):int -61 (-2.37% of base) : 111334.dasm - System.Net.Security.SafeDeleteContext:InitializeSecurityContext(byref,byref,System.String,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -60 (-2.28% of base) : 201260.dasm - System.Net.Security.SafeDeleteContext:AcceptSecurityContext(byref,byref,int,int,System.Net.Security.InputSecurityBuffers,byref,byref):int -19 (-2.22% of base) : 201214.dasm - System.Net.Security.NegotiateStreamPal:DecryptNtlm(System.Net.Security.SafeDeleteContext,System.Byte[],int,int,bool,byref,int):int -19 (-2.19% of base) : 197638.dasm - System.Net.Security.NegotiateStreamPal:VerifySignature(System.Net.Security.SafeDeleteContext,System.Byte[],int,int):int 26 total methods with Code Size differences (26 improved, 0 regressed), 0 unchanged -------------------------------------------------------------------------------- tests.pmi.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 116705 Total bytes of diff: 88209 Total bytes of delta: -28496 (-24.42% of base) diff is an improvement. Top file improvements (bytes): -251 : 88901.dasm (-41.35% of base) -251 : 88487.dasm (-41.35% of base) -238 : 88535.dasm (-42.20% of base) -238 : 88949.dasm (-42.20% of base) -237 : 136116.dasm (-45.84% of base) -237 : 136104.dasm (-45.84% of base) -232 : 136115.dasm (-42.80% of base) -232 : 136117.dasm (-42.80% of base) -232 : 136105.dasm (-42.80% of base) -231 : 88488.dasm (-43.10% of base) -231 : 88902.dasm (-43.10% of base) -228 : 87668.dasm (-40.50% of base) -228 : 87263.dasm (-42.46% of base) -228 : 87253.dasm (-40.50% of base) -228 : 87667.dasm (-42.46% of base) -228 : 87677.dasm (-40.50% of base) -228 : 87678.dasm (-42.46% of base) -228 : 87262.dasm (-40.50% of base) -228 : 87264.dasm (-40.50% of base) -228 : 87679.dasm (-40.50% of base) 224 total files with Code Size differences (224 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -251 (-41.35% of base) : 88901.dasm - testout1:Sub_Funclet_449():int -251 (-41.35% of base) : 88487.dasm - testout1:Sub_Funclet_449():int -238 (-42.20% of base) : 88535.dasm - testout1:Sub_Funclet_385():int -238 (-42.20% of base) : 88949.dasm - testout1:Sub_Funclet_385():int -237 (-45.84% of base) : 136116.dasm - testout1:Sub_Funclet_414():this -237 (-45.84% of base) : 136104.dasm - testout1:Sub_Funclet_452():this -232 (-42.80% of base) : 136115.dasm - testout1:Sub_Funclet_413():this -232 (-42.80% of base) : 136117.dasm - testout1:Sub_Funclet_415():this -232 (-42.80% of base) : 136105.dasm - testout1:Sub_Funclet_453():this -231 (-43.10% of base) : 88488.dasm - testout1:Sub_Funclet_450():int -231 (-43.10% of base) : 88902.dasm - testout1:Sub_Funclet_450():int -228 (-40.50% of base) : 87668.dasm - testout1:Sub_Funclet_453():int -228 (-42.46% of base) : 87263.dasm - testout1:Sub_Funclet_414():int -228 (-40.50% of base) : 87253.dasm - testout1:Sub_Funclet_453():int -228 (-42.46% of base) : 87667.dasm - testout1:Sub_Funclet_452():int -228 (-40.50% of base) : 87677.dasm - testout1:Sub_Funclet_413():int -228 (-42.46% of base) : 87678.dasm - testout1:Sub_Funclet_414():int -228 (-40.50% of base) : 87262.dasm - testout1:Sub_Funclet_413():int -228 (-40.50% of base) : 87264.dasm - testout1:Sub_Funclet_415():int -228 (-40.50% of base) : 87679.dasm - testout1:Sub_Funclet_415():int Top method improvements (percentages): -202 (-47.09% of base) : 88973.dasm - testout1:Sub_Funclet_410():int -202 (-47.09% of base) : 88560.dasm - testout1:Sub_Funclet_410():int -225 (-46.88% of base) : 99855.dasm - testout1:Sub_Funclet_449():int -225 (-46.68% of base) : 99441.dasm - testout1:Sub_Funclet_449():int -208 (-45.92% of base) : 99856.dasm - testout1:Sub_Funclet_450():int -237 (-45.84% of base) : 136116.dasm - testout1:Sub_Funclet_414():this -237 (-45.84% of base) : 136104.dasm - testout1:Sub_Funclet_452():this -208 (-45.71% of base) : 99442.dasm - testout1:Sub_Funclet_450():int -129 (-45.58% of base) : 88900.dasm - testout1:Sub_Funclet_448():int -129 (-45.58% of base) : 88486.dasm - testout1:Sub_Funclet_448():int -218 (-43.60% of base) : 136129.dasm - testout1:Sub_Funclet_427():this -218 (-43.60% of base) : 136155.dasm - testout1:Sub_Funclet_389():this -200 (-43.10% of base) : 136154.dasm - testout1:Sub_Funclet_388():this -200 (-43.10% of base) : 136128.dasm - testout1:Sub_Funclet_426():this -231 (-43.10% of base) : 88488.dasm - testout1:Sub_Funclet_450():int -231 (-43.10% of base) : 88902.dasm - testout1:Sub_Funclet_450():int -208 (-42.89% of base) : 98616.dasm - testout1:Sub_Funclet_453():int -208 (-42.89% of base) : 98625.dasm - testout1:Sub_Funclet_413():int -208 (-42.89% of base) : 98627.dasm - testout1:Sub_Funclet_415():int -232 (-42.80% of base) : 136115.dasm - testout1:Sub_Funclet_413():this 224 total methods with Code Size differences (224 improved, 0 regressed), 0 unchanged. -------------------------------------------------------------------------------- 

Fixes #49113.

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 7, 2021
@SingleAccretion SingleAccretion force-pushed the Remove-Top-Level-Bound-Checks-Nodes branch from 65dfb20 to e257c3b Compare March 7, 2021 09:35
@SingleAccretion SingleAccretion force-pushed the Remove-Top-Level-Bound-Checks-Nodes branch from e257c3b to 0841003 Compare March 7, 2021 15:13
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.

Looks good overall but left a few notes.

@SingleAccretion SingleAccretion force-pushed the Remove-Top-Level-Bound-Checks-Nodes branch from b6c7497 to fda5dee Compare March 9, 2021 18:54
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. Thanks!

@AndyAyersMS
Copy link
Member

OSX build timed out -- log doesn't show any hiccups, looks like the machine Mac-1615316275717 was just running the build very slowly. Am going to retry.

@SingleAccretion
Copy link
Contributor Author

SingleAccretion commented Mar 9, 2021

Looks like those checks were there for a good reason. Will investigate as soon as I have time.

Assert failure(PID 47507 [0x0000b993], Thread: 2454334 [0x25733e]): Assertion failed '((comma != nullptr) && comma->OperIs(GT_COMMA) && (comma->gtGetOp1() == check)) || (check == compCurStmt->GetRootNode())' in 'ILGEN_0x537f7b0:Method_0x323f83b5(double,byte,int,short,ubyte,double,short,ushort,double,long,int,long,ushort,long,int):float' during 'Early Value Propagation' (IL size 1860) 

Edit:
One test fails because of this curious extraction of side effects:

BB20 - Dead assignment has side effects... N018 ( 21, 31) [000845] -A-XG---R--- * ASG int N017 ( 1, 1) [000836] D------N---- +--* LCL_VAR int V02 arg2 N016 ( 21, 31) [001639] *A-XG------- \--* IND int N015 ( 18, 29) [001636] -A-XG------- \--* COMMA byref N004 ( 4, 12) [001624] -A--G---R--- +--* ASG ref N003 ( 1, 1) [001623] D------N---- | +--* LCL_VAR ref V123 tmp93 N002 ( 4, 12) [000838] n---G------- | \--* IND ref N001 ( 2, 10) [001637] H----------- | \--* CNS_INT(h) long 0x299995e2c48 static Fseq[field_0x6] N014 ( 14, 17) [001635] ---XG------- \--* COMMA byref N008 ( 8, 11) [001629] ---X-------- +--* ARR_BOUNDS_CHECK_Rng void N005 ( 1, 1) [000841] ------------ | +--* CNS_INT int 0 N007 ( 3, 3) [001628] ---X-------- | \--* ARR_LENGTH int N006 ( 1, 1) [001625] ------------ | \--* LCL_VAR ref V123 tmp93 N013 ( 6, 6) [001638] ----G------- \--* ADDR byref N012 ( 4, 4) [000842] a---G--N---- \--* IND int N011 ( 2, 2) [001634] -------N---- \--* ADD byref N009 ( 1, 1) [001626] ------------ +--* LCL_VAR ref V123 tmp93 N010 ( 1, 1) [001633] ------------ \--* CNS_INT long 16 Fseq[#FirstElem] top level assign Extracted side effects list... [001749] -A-XG------- * COMMA void N004 ( 4, 12) [001624] -A--G---R--- +--* ASG ref N003 ( 1, 1) [001623] D------N---- | +--* LCL_VAR ref V123 tmp93 N002 ( 4, 12) [000838] n---G------- | \--* IND ref N001 ( 2, 10) [001637] H----------- | \--* CNS_INT(h) long 0x299995e2c48 static Fseq[field_0x6] N008 ( 8, 11) [001629] ---X-------- \--* ARR_BOUNDS_CHECK_Rng void N005 ( 1, 1) [000841] ------------ +--* CNS_INT int 0 N007 ( 3, 3) [001628] ---X-------- \--* ARR_LENGTH int N006 ( 1, 1) [001625] ------------ \--* LCL_VAR ref V123 tmp93 
@SingleAccretion
Copy link
Contributor Author

To summarize, there are cases today where TYP_VOID COMMAs are created that act like statements (contain only statement-like expressions), and the second operand in them can be anything, including range checks. This actually can cause CQ problems (I believe the impact to be very limited however). I will create an issue with details on the circumstances of how these COMMAs are created.

@AndyAyersMS
Copy link
Member

Ah right, that makes sense, various utilities can create side effect lists like the one you see.

Seems like an unused array access might be enough to hit this:

class X { int[] a; void F(int i) { _ = a[i]; } } 
@SingleAccretion
Copy link
Contributor Author

Indeed...

This simple method hits it due to how the INDEX is expanded:

public static void Problem() { static int[] Static2() => new int[100]; int n = Static2()[0]; }
fgMorphTree BB01, STMT00001 (before) [000005] --CXG------- * COMMA void [000003] --CXG------- +--* INDEX int [000009] --CXG------- | +--* CALL help ref HELPER.CORINFO_HELP_NEWARR_1_VC [000008] H----------- arg0 | | +--* CNS_INT(h) long 0xd1ffab1e class [000007] ------------ arg1 | | \--* CNS_INT long 100 [000002] ------------ | \--* CNS_INT int 0 [000004] ------------ \--* NOP void fgMorphTree BB01, STMT00001 (after) [000005] -ACXG+------ * COMMA void [000027] -ACXG------- +--* COMMA void [000012] -ACXG+------ | +--* ASG ref [000011] D----+-N---- | | +--* LCL_VAR ref V01 tmp1 [000009] --CXG+------ | | \--* CALL help ref HELPER.CORINFO_HELP_NEWARR_1_VC [000008] H----+------ arg0 in rcx | | +--* CNS_INT(h) long 0xd1ffab1e class [000007] -----+------ arg1 in rdx | | \--* CNS_INT long 100 [000017] ---X-+------ | \--* ARR_BOUNDS_CHECK_Rng void [000002] -----+------ | +--* CNS_INT int 0 [000016] ---X-+------ | \--* ARR_LENGTH int [000013] -----+------ | \--* LCL_VAR ref V01 tmp1 [000004] -----+------ \--* NOP void 

(And this ARR_BOUNDS_CHECK_Rng will not be eliminated, emitting mov eax, 100; test eax, eax)

Other cases will hit it for other reasons (like the dead assignment elimination, most likely more).

I suppose there's not much actionable here, as the cases when it happens seem to be exceedingly rare (there were no asserts seen in the libs tests), and it is handled correctly. One idea I had in connection to this is "why not expand to proper Statements when we can, like in the dead assignment optimization", but I don't know whether that'd be advantageous or detrimental to downstream analysis as it exists today.

@nathan-moore
Copy link
Contributor

You might want to also check out #10686 and #39156

@AndyAyersMS
Copy link
Member

I'm going to go ahead and merge this. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 23, 2021
@SingleAccretion SingleAccretion deleted the Remove-Top-Level-Bound-Checks-Nodes branch June 8, 2021 04:14
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

4 participants