Remove top-level range check nodes #49271
Conversation
ghost
added
the
area-CodeGen-coreclr
SingleAccretion
force-pushed
the
Remove-Top-Level-Bound-Checks-Nodes
branch
from
65dfb20 to
e257c3b
Compare
SingleAccretion
force-pushed
the
Remove-Top-Level-Bound-Checks-Nodes
branch
from
e257c3b to
0841003
Compare
SingleAccretion
force-pushed
the
Remove-Top-Level-Bound-Checks-Nodes
branch
from
b6c7497 to
fda5dee
Compare
|
OSX build timed out -- log doesn't show any hiccups, looks like the machine |
|
Looks like those checks were there for a good reason. Will investigate as soon as I have time. Edit: 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 |
|
To summarize, there are cases today where |
|
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: |
|
Indeed... This simple method hits it due to how the 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 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 |
|
I'm going to go ahead and merge this. Thanks! |
See #49113 for context.
The change is two-fold: refactoring of
optRemoveRangeCheckto handle both types of checks ("flattened"COMMAs and regularCOMMAs) and threading the support through the relevant phases.I decided to concentrate the new logic in
optRemoveRangeCheckinstead 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
optRemoveRangeCheckfor code that does know what type of check it is dealing with.This fixes the original reproduction:
It also works for a non-zero constant (the change in range check elimination is responsible for that):
There are some diffs as well (mostly in CoreLib):
The diffs
Fixes #49113.