- Notifications
You must be signed in to change notification settings - Fork 5.3k
Remove top-level range check nodes #49271
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
Remove top-level range check nodes #49271
Conversation
65dfb20 to e257c3b Compare e257c3b to 0841003 Compare
AndyAyersMS left a comment
There was a problem hiding this 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.
b6c7497 to fda5dee Compare
AndyAyersMS left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
| 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.