Removed range checks have ordered side effects (#78554) #78561

jakobbotsch · 2022-11-18T15:49:14Z

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?

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.

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).

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.

So, at creation of BOUNDS_CHECK_Rng ? Or do you still think at the original creation of the IND (during the import stages)

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).

It looks like the morph fix causes something else to break - so, yes, looks like it needs some additional checks.

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;

?

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 2

From 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.

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_SIDEEFF here.

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.

-Original file line number
+Diff line change
@@ Expand Up @@
         {
             // 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;
         }
         gtUpdateSideEffects(stmt, tree);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,30 @@
+    // Licensed to the .NET Foundation under one or more agreements.
+    // The .NET Foundation licenses this file to you under the MIT license.
+    using System.Runtime.CompilerServices;
+    public class Runtime_78554
+    {
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static void Consume(uint op)
+        {
+            return;
+        }
+        [MethodImplAttribute(MethodImplOptions.NoInlining)]
+        static void ArrayIndexConsume(uint[] a, uint i)
+        {
+            if (i < a.Length)
+            {
+               i = a[i];
+            }
+            Consume(i);
+        }
+        public static int Main()
+        {
+            var arr = new uint[] { 1, 42, 3000 };
+            ArrayIndexConsume(arr, 0xffffffff);
+            return 100;
+        }
+    }

-Original file line number
+Diff line change
@@ -0,0 +1,13 @@
+    <Project Sdk="Microsoft.NET.Sdk">
+      <PropertyGroup>
+        <OutputType>Exe</OutputType>
+        <Optimize>True</Optimize>
+      </PropertyGroup>
+      <ItemGroup>
+        <Compile Include="$(MSBuildProjectName).cs" />
+        <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
+        <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />
+        <CLRTestEnvironmentVariable Include="DOTNET_JitStressModeNames" Value="STRESS_IF_CONVERSION_COST" />
+      </ItemGroup>
+    </Project>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed range checks have ordered side effects (#78554) #78561

Diff view

Diff view

There are no files selected for viewing

jakobbotsch Nov 18, 2022

a74nh Nov 18, 2022

jakobbotsch Nov 18, 2022

a74nh Nov 18, 2022

a74nh Nov 18, 2022

jakobbotsch Nov 18, 2022

a74nh Nov 18, 2022

jakobbotsch Nov 21, 2022

jakobbotsch Nov 22, 2022

a74nh Nov 22, 2022

Removed range checks have ordered side effects (#78554) #78561

Removed range checks have ordered side effects (#78554) #78561

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment