Removed range checks have ordered side effects (#78554) #78561
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBounds checks can be removed due to assertion propagation stating that the bounds check will always pass. Once removed, nodes that constrained by the bounds check cannot be moved elsewhere as that might break the assertion. This prevents If Conversion hoisting array loads outside of an
|
| @@ -9216,6 +9216,8 @@ GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma, | |||
| { | |||
| // TODO-CQ: We should also remove the GT_COMMA, but in any case we can no longer CSE the GT_COMMA. | |||
| tree->gtFlags |= GTF_DONT_CSE; | |||
| // Ensure the other side of the comma can't be moved outside the checked range. | |||
| tree->gtGetOp2()->gtFlags |= GTF_ORDER_SIDEEFF; | |||
There was a problem hiding this comment.
I would've expected we would set this in the same place we set GTF_IND_NONFAULTING on the indirection and remove its exception flags. Where do we do that?
There was a problem hiding this comment.
That would be in morph.cpp....
The IND is moved after the BOUNDS_CHECK_Rng and then it removes the GTF_EXCEPT flag:
// if this was a non-faulting indir, clear GTF_EXCEPT,
// unless we inherit it from the addr.
//
if (((treeFlags & GTF_IND_NONFAULTING) != 0) && ((addr->gtFlags & GTF_EXCEPT) == 0))
{
op1->gtFlags &= ~GTF_EXCEPT;
}
I'll move the change to here.
There was a problem hiding this comment.
I think it fits better where we set GTF_IND_NONFAULTING. There are many GTF_IND_NONFAULTING indirs that are safe to reorder (in particular handles).
There was a problem hiding this comment.
The original place the IND gets marked as non faulting is when it gets created:
STMT00003 ( 0x008[E-] ... ??? )
[000018] -A-XG------ * ASG int
[000017] D------N--- +--* LCL_VAR int V01 arg1
[000016] n--XG------ \--* IND int
[000015] ---XG------ \--* INDEX_ADDR byref int[]
[000013] ----------- +--* LCL_VAR ref V00 arg0
[000014] ----------- \--* LCL_VAR int V01 arg1
At that point it's still safe to move around. (I think).
The INDEX_ADDR node 15 gets turned into a COMMA with bounds check (without changing the IND)
[000030] ---X------- * COMMA byref
[000022] ---X------- +--* BOUNDS_CHECK_Rng void
[000014] ----------- | +--* LCL_VAR int V01 arg1
[000021] ---X------- | \--* ARR_LENGTH int
[000013] ----------- | \--* LCL_VAR ref V00 arg0
[000029] ----------- \--* ARR_ADDR byref int[]
[000028] ----------- \--* ADD byref
[000027] ----------- +--* ADD byref
[000019] ----------- | +--* LCL_VAR ref V00 arg0
[000026] ----------- | \--* CNS_INT long 16
[000025] ----------- \--* MUL long
[000023] ---------U- +--* CAST long <- uint
[000020] ----------- | \--* LCL_VAR int V01 arg1
[000024] -------N--- \--* CNS_INT long 4
Then finally the IND is moved after the bounds check and the exception flag is dropped:
[000018] -A-XG+----- * ASG int
[000017] D----+-N--- +--* LCL_VAR int V01 arg1
[000030] ---XG+----- \--* COMMA int
[000022] ---X-+----- +--* BOUNDS_CHECK_Rng void
[000014] -----+----- | +--* LCL_VAR int V01 arg1
[000021] ---X-+----- | \--* ARR_LENGTH int
[000013] -----+----- | \--* LCL_VAR ref V00 arg0
[000031] n---G+----- \--* IND int
[000029] -----+----- \--* ARR_ADDR byref int[]
[000028] -----+----- \--* ADD byref
[000027] -----+----- +--* ADD byref
[000019] -----+----- | +--* LCL_VAR ref V00 arg0
[000026] -----+----- | \--* CNS_INT long 16
[000025] -----+----- \--* LSH long
[000023] -----+---U- +--* CAST long <- uint
[000020] -----+----- | \--* LCL_VAR int V01 arg1
[000024] -----+-N--- \--* CNS_INT long 2
It was this final step I was adding the order side effect.
There was a problem hiding this comment.
So, at creation of BOUNDS_CHECK_Rng ? Or do you still think at the original creation of the IND (during the import stages)
There was a problem hiding this comment.
You are right. I think it makes sense to do it at the point you were considering in morph.cpp. We'll probably need to check that this only hits the expected cases (the transformation seems a bit more general than just for bounds checks, but being conservative about it will hopefully be ok).
There was a problem hiding this comment.
It looks like the morph fix causes something else to break - so, yes, looks like it needs some additional checks.
There was a problem hiding this comment.
What issues did you hit doing this in morph? I assume the diff was
diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp
index c1b827e623f..309a34716c4 100644
--- a/src/coreclr/jit/morph.cpp
+++ b/src/coreclr/jit/morph.cpp
@@ -10690,6 +10690,7 @@ DONE_MORPHING_CHILDREN:
if (((treeFlags & GTF_IND_NONFAULTING) != 0) && ((addr->gtFlags & GTF_EXCEPT) == 0))
{
op1->gtFlags &= ~GTF_EXCEPT;
+ op1->gtFlags |= GTF_ORDER_SIDEEFF;
}
op1->gtFlags |= treeFlags & GTF_GLOB_REF;?
There was a problem hiding this comment.
I had a more in depth look at this and discussed it with @SingleAccretion.
Essentially the problem here is that morphing of INDEX_ADDR nodes turns a value dependency into a flow dependency by introducing a comma. Before that, we have:
[000016] n--XG------ └──▌ IND int
[000015] ---XG------ └──▌ INDEX_ADDR byref int[]
[000013] ----------- ├──▌ LCL_VAR ref V00 arg0
[000014] ----------- └──▌ LCL_VAR int V01 arg1 after that, we end up with
[000016] n--XG------ ▌ IND int
[000030] ---X-+----- └──▌ COMMA byref
[000022] ---X-+----- ├──▌ BOUNDS_CHECK_Rng void
[000014] -----+----- │ ├──▌ LCL_VAR int V01 arg1
[000021] ---X-+----- │ └──▌ ARR_LENGTH int
[000013] -----+----- │ └──▌ LCL_VAR ref V00 arg0
[000029] -----+----- └──▌ ARR_ADDR byref int[]
[000028] -----+----- └──▌ ADD byref
[000027] -----+----- ├──▌ ADD byref
[000019] -----+----- │ ├──▌ LCL_VAR ref V00 arg0
[000026] -----+----- │ └──▌ CNS_INT long 16
[000025] -----+----- └──▌ LSH long
[000023] -----+---U- ├──▌ CAST long <- uint
[000020] -----+----- │ └──▌ LCL_VAR int V01 arg1
[000024] -----+-N--- └──▌ CNS_INT long 2From an IR standpoint we are already in trouble by not having introduced GTF_ORDER_SIDEEFF here. The reason is that we do not model "access violation" as a side effect of indirections; only NullReferenceException is a modelled side effect. So now, or later, the JIT is free to notice that ARR_ADDR is never null and consider the indirection to be side effect free (which is what happens).
While there is sort of an explicit dependency between [000030] and [000016] in HIR, it is not the case in LIR, and even if we didn't do the IND(COMMA(x, ..., z)) -> COMMA(x, ..., IND(z)) transformation here the backend would not be able to check interference properly after we have done this transformation.
So that suggests to me that we should be setting GTF_ORDER_SIDEEFF on the bounds check here (in other words, op1 of the comma, since we turned the value dependency into a flow dependency by introducing it).
I think this will fix the issue in practice, but that's primarily because the GTF_ORDER_SIDEEFF flag will be propagated upwards to the IND node. It might not fix the problem if the IR shape was not very particular here, but I think this is sufficient for our purposes for now.
For an ideal long term fix I think we have several other places that ought to introduce GTF_ORDER_SIDEEFF on bounds checks (by which I mean either BOUNDS_CHECK_Rng nodes or user-written bounds checks) when acting on them, particularly some in loop cloning and assertion prop. The assertion prop one is difficult as we do not store the original node in assertions; conservatively we could mark any bounds check that generated an assertion with this flag, but even that would require some additional work we do not expect to introduce flags at this point.
I think, in addition, if-conversion needs to also check that the condition does not have GTF_ORDER_SIDEEFF.
Do you want to submit these changes or do you want me to handle it? I expect that there might be some work involved in checking and dealing with regressions.
There was a problem hiding this comment.
I ended up with this in morph.cpp:
> if (((treeFlags & GTF_IND_NONFAULTING) != 0) && ((addr->gtFlags & GTF_EXCEPT) == 0)
> && ((op1->gtFlags & GTF_EXCEPT) != 0))
> {
> op1->gtFlags &= ~GTF_EXCEPT;
> op1->gtFlags |= GTF_ORDER_SIDEEFF;
> }
Checking GTF_EXCEPT was set on op1 was essential.
That mostly worked, except for about 6 failures in spmi replay.
src/coreclr/jit/fgdiagnostic.cpp (3202) - Assertion failed '!"Missing flags on tree"' in 'ILGEN_0xedada940:Method_0x7f75():int' during 'Morph - Global' (IL size 142; hash 0x631c06ca; FullOpts)
[000045] -ACXG+----- * ASG long
[000101] -ACXG+-N--- +--* COMMA long
[000086] -ACXG+----- | +--* ASG ref
[000085] D----+-N--- | | +--* LCL_VAR ref V10 tmp3
[000039] --CXG+----- | | \--* CALL help ref HELPER.CORINFO_HELP_NEWARR_1_VC
[000037] H----+----- arg0 in x0 | | +--* CNS_INT(h) long 0x7f56585778 class
[000038] -----+----- arg1 in x1 | | \--* CNS_INT long 26
[000100] --CXG+-N--- | \--* COMMA long
[000104] --CXG+----- | +--* CALL help void HELPER.CORINFO_HELP_OVERFLOW
[000103] -----+----- arg0 in x0 | | \--* CNS_INT int 1
[000109] --CXG+-N--- | \--* COMMA long
[000108] --CXG+----- | +--* CALL help void HELPER.CORINFO_HELP_OVERFLOW
[000107] -----+----- arg0 in x0 | | \--* CNS_INT int 1
[000112] n---G+-N--- | \--* IND long
[000106] ----------- | \--* CNS_INT byref 0
[000042] -----+----- \--* CNS_INT long 0x46fa6f8d4e707118
In the above graph, 000100 is missing the GTF_ORDER_SIDEEFF. And I suspect 000101 and 000045 is too. I think we have the original ASG node at being passed into the morph function, so should be fairly easy to propagate.
Looking at your analysis:
From an IR standpoint we are already in trouble by not having introduced
GTF_ORDER_SIDEEFFhere.
I think I had assumed that was ok, because the bounds check and arr addr were connected by the comma. But what your saying makes sense.
Do you want to submit these changes or do you want me to handle it? I expect that there might be some work involved in checking and dealing with regressions.
I'm mostly away from my computer this week until Thursday. So, if you've got a solution, then I'm happy for you to go ahead with it. And thanks for looking into this.
|
Closing this because #78698 is a more complete fix. (Thanks @jakobbotsch) |
Bounds checks can be removed due to assertion propagation stating that the bounds check will always pass. Once removed, nodes that constrained by the bounds check cannot be moved elsewhere as that might break the assertion.
This prevents If Conversion hoisting array loads outside of an
Ifstatement