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

I decided to concentrate the new logic inoptRemoveRangeCheck 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 newoptRemoveRangeCheck for code that does know what type of check it is dealing with.

This fixes the original reproduction:

publicstaticvoidCheckZero(Span<int>a){staticintValue()=>42;if(!a.IsEmpty){a[0]=Value();}}

G_M7183_IG02:movrax, bword ptr[rcx]movedx, dword ptr[rcx+8]G_M7183_IG03:testedx,edxjbe      SHORT G_M7183_IG05G_M7183_IG04:mov      dword ptr[rax],42G_M7183_IG05:ret

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

publicstaticvoidCheckOne(Span<int>a){staticintValue()=>42;if(a.Length>1){a[0]=Value();}}

movrax, bword ptr[rcx]movedx, dword ptr[rcx+8]G_M7183_IG03:cmpedx,1jle      SHORT G_M7183_IG05G_M7183_IG04:mov      dword ptr[rax],42G_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 jitSummary of Code Size diffs:(Lower is better)Total bytes of base: 33745766Total bytes of diff: 33745610Total 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.mchSummary of Code Size diffs:(Lower is better)Total bytes of base: 7261Total bytes of diff: 7078Total 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]):ubyteTop 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):this7 total methods with Code Size differences (7 improved, 0 regressed), 0 unchanged.--------------------------------------------------------------------------------libraries.pmi.windows.x64.checked.mchSummary of Code Size diffs:(Lower is better)Total bytes of base: 32144Total bytes of diff: 31341Total 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):int26 total methods with Code Size differences (26 improved, 0 regressed), 0 unchanged--------------------------------------------------------------------------------tests.pmi.windows.x64.checked.mchSummary of Code Size diffs:(Lower is better)Total bytes of base: 116705Total bytes of diff: 88209Total 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():intTop 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():this224 total methods with Code Size differences (224 improved, 0 regressed), 0 unchanged.--------------------------------------------------------------------------------

Fixes#49113.

Refactored optRemoveRangeCheck to handle top-level standalone range c…

adc7dde

…heck nodes

ghost added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label

Mar 7, 2021

SingleAccretion force-pushed theRemove-Top-Level-Bound-Checks-Nodes branch from65dfb20 toe257c3bCompare

March 7, 2021 09:35

SingleAccretion closed this

Mar 7, 2021

SingleAccretion reopened this

Mar 7, 2021

Added support for eliminating top-level standalone range checks

0841003

SingleAccretion force-pushed theRemove-Top-Level-Bound-Checks-Nodes branch frome257c3b to0841003Compare

March 7, 2021 15:13

AndyAyersMS reviewed

Mar 9, 2021

View reviewed changes

Copy link

Member

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.

src/coreclr/jit/rangecheck.cpp OutdatedShow resolvedHide resolved

src/coreclr/jit/optimizer.cpp OutdatedShow resolvedHide resolved

src/coreclr/jit/optimizer.cppShow resolvedHide resolved

src/coreclr/jit/earlyprop.cppShow resolvedHide resolved

SingleAccretion added4 commits

March 9, 2021 10:03

Add missing information to the function headers

b5d1b2b

Expand an assertion

71b611e

Avoid redundant checks

44bf913

Assert on invalid IR

fda5dee

SingleAccretion force-pushed theRemove-Top-Level-Bound-Checks-Nodes branch fromb6c7497 tofda5deeCompare

March 9, 2021 18:54

AndyAyersMS approved these changes

Mar 9, 2021

View reviewed changes

Copy link

Member

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!

Copy link

Member

AndyAyersMS commentedMar 9, 2021

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

Copy link

ContributorAuthor

SingleAccretion commentedMar 9, 2021•
edited
Loading

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       intN017 (1,1) [000836]D------N----+--*LCL_VAR   intV02 arg2N016 (21,31) [001639]*A-XG-------\--*IND       intN015 (18,29) [001636]-A-XG-------\--*COMMA     byrefN004 (4,12) [001624]-A--G---R---+--*ASG       refN003 (1,1) [001623]D------N----|+--*LCL_VAR   refV123 tmp93N002 (4,12) [000838] n---G-------|\--*IND       refN001 (2,10) [001637]H-----------|\--*CNS_INT(h) long0x299995e2c48 staticFseq[field_0x6]N014 (14,17) [001635]---XG-------\--*COMMA     byrefN008 (8,11) [001629]---X--------+--*ARR_BOUNDS_CHECK_Rng voidN005 (1,1) [000841]------------|+--*CNS_INT   int0N007 (3,3) [001628]---X--------|\--*ARR_LENGTH intN006 (1,1) [001625]------------|\--*LCL_VAR   refV123 tmp93N013 (6,6) [001638]----G-------\--*ADDR      byrefN012 (4,4) [000842] a---G--N----\--*IND       intN011 (2,2) [001634]-------N----\--*ADD       byrefN009 (1,1) [001626]------------+--*LCL_VAR   refV123 tmp93N010 (1,1) [001633]------------\--*CNS_INT   long16Fseq[#FirstElem]top level assignExtracted side effects list...               [001749]-A-XG-------*COMMA     voidN004 (4,12) [001624]-A--G---R---+--*ASG       refN003 (1,1) [001623]D------N----|+--*LCL_VAR   refV123 tmp93N002 (4,12) [000838] n---G-------|\--*IND       refN001 (2,10) [001637]H-----------|\--*CNS_INT(h) long0x299995e2c48 staticFseq[field_0x6]N008 (8,11) [001629]---X--------\--*ARR_BOUNDS_CHECK_Rng voidN005 (1,1) [000841]------------+--*CNS_INT   int0N007 (3,3) [001628]---X--------\--*ARR_LENGTH intN006 (1,1) [001625]------------\--*LCL_VAR   refV123 tmp93

Relax the assert to allow TYP_VOID COMMAs with bounds checks as the s…

fa1e5f5

…econd operand

Copy link

ContributorAuthor

SingleAccretion commentedMar 10, 2021

To summarize, there are cases today whereTYP_VOIDCOMMAs 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 theseCOMMAs are created.

Copy link

Member

AndyAyersMS commentedMar 10, 2021

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]; }}

Copy link

ContributorAuthor

SingleAccretion commentedMar 10, 2021

Indeed...

This simple method hits it due to how theINDEX is expanded:

publicstaticvoidProblem(){staticint[]Static2()=>newint[100];intn=Static2()[0];}

fgMorphTreeBB01,STMT00001 (before)               [000005]--CXG-------*COMMA     void                 [000003]--CXG-------+--*INDEX     int                  [000009]--CXG-------|+--*CALL help refHELPER.CORINFO_HELP_NEWARR_1_VC               [000008]H----------- arg0||+--*CNS_INT(h) long0xd1ffab1eclass               [000007]------------ arg1||\--*CNS_INT   long100               [000002]------------|\--*CNS_INT   int0               [000004]------------\--*NOP       void  fgMorphTreeBB01,STMT00001 (after)               [000005]-ACXG+------*COMMA     void                 [000027]-ACXG-------+--*COMMA     void                 [000012]-ACXG+------|+--*ASG       ref                  [000011]D----+-N----||+--*LCL_VAR   refV01 tmp1                        [000009]--CXG+------||\--*CALL help refHELPER.CORINFO_HELP_NEWARR_1_VC               [000008]H----+------ arg0 in rcx||+--*CNS_INT(h) long0xd1ffab1eclass               [000007]-----+------ arg1 in rdx||\--*CNS_INT   long100               [000017]---X-+------|\--*ARR_BOUNDS_CHECK_Rng void                 [000002]-----+------|+--*CNS_INT   int0               [000016]---X-+------|\--*ARR_LENGTH int                  [000013]-----+------|\--*LCL_VAR   refV01 tmp1                        [000004]-----+------\--*NOP       void

(And thisARR_BOUNDS_CHECK_Rng will not be eliminated, emittingmov 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 properStatements 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.