Ordering issues with DYN_BLK nodes

[SingleAccretion]

The ASG(DYN_BLK, IND) representation makes it (almost) impossible, by design, to maintain the proper ordering.

Dynamic blocks are imported for two instructions: cpblk and initblk. It is the first one that has issues. The order in which the operands are pushed onto the stack (and thus the proper evaluation order for the resulting tree) is as follows:

push dst
push src
push size

Yet, we import the following code:

public static void CpBlk(void* dst, void* src, uint count)
{
    IL.Emit.Ldarg(0);
    IL.Emit.Ldarg(1);
    IL.Emit.Ldarg(2);
    IL.Emit.Cpblk();
}

As the following tree:

***** BB01
STMT00000 ( 0x000[E-] ... 0x004 )
               [000005] -A-XG-------              *  ASG       struct (copy)
               [000003] ---XG--N----              +--*  DYN_BLK   struct
               [000000] ------------              |  +--*  LCL_VAR   long   V00 arg0  // dst   
               [000002] ------------              |  \--*  LCL_VAR   int    V02 arg2  // size
               [000004] -------N----              \--*  IND       struct
               [000001] ------------                 \--*  LCL_VAR   long   V01 arg1  // src

The order of evaluation for size and src has been swapped.

This does not matter for locals of course, but let's see what happens if we change our example to something like:

public static void CpBlkSimpleSideEffects(void* dst, void** pSrc, long count)
{
    IL.Emit.Ldarg(0);
    IL.Emit.Ldarg(1);
    IL.Emit.Ldind_I();
    IL.Emit.Ldarg(2);
    IL.Emit.Conv_Ovf_U4();
    IL.Emit.Cpblk();
}

Actually - this sample ends up generating correct code! And the reason for this is the fact that gtSetEvalOrder swaps the assignment order, which it always does for LHS indirections with addresses represented by locals:

runtime/src/coreclr/jit/gentree.cpp

Lines 4350 to 4355 in 90773ac

	if (op1Val->AsIndir()->Addr()->IsLocalAddrExpr())
	{
	bReverseInAssignment = true;
	tree->gtFlags |= GTF_REVERSE_OPS;
	break;
	}

So we end up with this threaded code, which has correct evaluation order:

N008 ( 16, 15) [000007] -A-XG---R---              *  ASG       struct (copy)
N007 (  9, 10) [000005] D--XG--N----              +--*  DYN_BLK   struct
N004 (  1,  1) [000000] ------------              |  +--*  LCL_VAR   long   V00 arg0         u:1 (last use)
N006 (  8,  9) [000004] ---X--------              |  \--*  CAST_ovfl int <- uint <- long
N005 (  1,  1) [000003] ------------              |     \--*  LCL_VAR   long   V02 arg2         u:1 (last use)
N003 (  6,  4) [000006] ---XG--N----              \--*  IND       struct
N002 (  3,  2) [000002] *--XG-------                 \--*  IND       long  
N001 (  1,  1) [000001] ------------                    \--*  LCL_VAR   long   V01 arg1         u:1 (last use)

Of course, things don't work out quite so well if we have a more complex expression for the destination address:

public static void CpBlkSideEffects(void** dst, void*[] src, long count)
{
    IL.Emit.Ldarg(0);
    IL.Emit.Ldind_I();
    IL.Emit.Ldarg(1);
    IL.Emit.Ldc_I4(0);
    IL.Emit.Ldelem_I();
    IL.Emit.Ldarg(2);
    IL.Emit.Conv_Ovf_U4();
    IL.Emit.Cpblk();
}

And this program happily throws OverflowException, while it should have thrown IndexOutOfRangeException:

int  dstSrc = 0;
int* dst    = &dstSrc;
var  src    = new void*[0];
CpBlkSideEffects((void**)&dst, src, long.MaxValue);

Fortunately:

1. This bug is only reproducible in pure IL, as the inliner will spill side-effecting arguments for Unsafe.CopyBlock.
2. It has been there for a very long time.
3. No one uses cpblk.

So we're in no rush to fix it (which, I suspect, would involve getting rid of DYN_BLK and importing STORE_DYN_BLK directly).

[ghost]

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The ASG(DYN_BLK, IND) representation makes it (almost) impossible, by design, to maintain the proper ordering.

Dynamic blocks are imported for two instructions: cpblk and initblk. It is the first one that has issues. The order in which the operands are pushed onto the stack (and thus the proper evaluation order for the resulting tree) is as follows:

push dst
push src
push size

Yet, we import the following code:

public static void CpBlk(void* dst, void* src, uint count)
{
    IL.Emit.Ldarg(0);
    IL.Emit.Ldarg(1);
    IL.Emit.Ldarg(2);
    IL.Emit.Cpblk();
}

As the following tree:

***** BB01
STMT00000 ( 0x000[E-] ... 0x004 )
               [000005] -A-XG-------              *  ASG       struct (copy)
               [000003] ---XG--N----              +--*  DYN_BLK   struct
               [000000] ------------              |  +--*  LCL_VAR   long   V00 arg0  // dst   
               [000002] ------------              |  \--*  LCL_VAR   int    V02 arg2  // size
               [000004] -------N----              \--*  IND       struct
               [000001] ------------                 \--*  LCL_VAR   long   V01 arg1  // src

The order of evaluation for size and src has been swapped.

This does not matter for locals of course, but let's see what happens if we change our example to something like:

public static void CpBlkSimpleSideEffects(void* dst, void** pSrc, long count)
{
    IL.Emit.Ldarg(0);
    IL.Emit.Ldarg(1);
    IL.Emit.Ldind_I();
    IL.Emit.Ldarg(2);
    IL.Emit.Conv_Ovf_U4();
    IL.Emit.Cpblk();
}

Actually - this sample ends up generating correct code! And the reason for this is the fact that gtSetEvalOrder swaps the assignment order, which it always does for LHS indirections with addresses represented by locals:

runtime/src/coreclr/jit/gentree.cpp

Lines 4350 to 4355 in 90773ac

	if (op1Val->AsIndir()->Addr()->IsLocalAddrExpr())
	{
	bReverseInAssignment = true;
	tree->gtFlags |= GTF_REVERSE_OPS;
	break;
	}

So we end up with this threaded code, which has correct evaluation order:

N008 ( 16, 15) [000007] -A-XG---R---              *  ASG       struct (copy)
N007 (  9, 10) [000005] D--XG--N----              +--*  DYN_BLK   struct
N004 (  1,  1) [000000] ------------              |  +--*  LCL_VAR   long   V00 arg0         u:1 (last use)
N006 (  8,  9) [000004] ---X--------              |  \--*  CAST_ovfl int <- uint <- long
N005 (  1,  1) [000003] ------------              |     \--*  LCL_VAR   long   V02 arg2         u:1 (last use)
N003 (  6,  4) [000006] ---XG--N----              \--*  IND       struct
N002 (  3,  2) [000002] *--XG-------                 \--*  IND       long  
N001 (  1,  1) [000001] ------------                    \--*  LCL_VAR   long   V01 arg1         u:1 (last use)

Of course, things don't work out quite so well if we have a more complex expression for the destination address:

public static void CpBlkSideEffects(void** dst, void*[] src, long count)
{
    IL.Emit.Ldarg(0);
    IL.Emit.Ldind_I();
    IL.Emit.Ldarg(1);
    IL.Emit.Ldc_I4(0);
    IL.Emit.Ldelem_I();
    IL.Emit.Ldarg(2);
    IL.Emit.Conv_Ovf_U4();
    IL.Emit.Cpblk();
}

And this program happily throws OverflowException, while it should have thrown IndexOutOfRangeException:

int  dstSrc = 0;
int* dst    = &dstSrc;
var  src    = new void*[0];
CpBlkSideEffects((void**)&dst, src, long.MaxValue);

Fortunately:

1. This bug is only reproducible in pure IL, as the inliner will spill side-effecting arguments for Unsafe.CopyBlock.
2. It has been there for a very long time.
3. No one uses cpblk.

So we're in no rush to fix it (which, I suspect, would involve getting rid of DYN_BLK and importing STORE_DYN_BLK directly).

Author:	SingleAccretion
Assignees:	-
Labels:	bug, area-CodeGen-coreclr
Milestone:	-

[JulieLeeMSFT]

CC @dotnet/jit-contrib