Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix InitializeArray intrinsic must always be expanded for CoreRT. #14401

Merged
merged 1 commit into from
Oct 10, 2017

Conversation

sandreenko
Copy link

@sandreenko sandreenko commented Oct 10, 2017

The issue is dotnet/corert#4690.

The problem is the same as the old one(#10215), we do spill because reach the max tree size.

impInitializeArrayIntrinsic relies that the last stmt is the helper call.
We violate it if MAX_TREE spill happens before dbg spill on the stmt boundaries. We ends up with the wrong stmt order and can't import the intrinsic.

It works with optimization because we force to spill after 'dup' that are not 'elementary' and the right spills happen before we spill because of MAX_TREE_SIZE.

The good example:

    [ 1]   7 (0x007) dup
    [ 2]   8 (0x008) ldc.i4.0 0
    [ 3]   9 (0x009) ldc.i4.s 16
    [ 4]  11 (0x00b) newarr 01000047
*****************************************SPILL FOR DBG CODE*********************************************(
 spill tree 9
lvaGrabTemp returning 1 (V01 tmp1) called for impSpillStackEnsure.
assign tree 9 to temp 1


               [000015] ------------             *  STMT      void  (IL 0x007...  ???)
               [000009] ------------             |  /--*  CNS_INT   int    0
               [000014] -A----------             \--*  ASG       int
               [000013] D------N----                \--*  LCL_VAR   int    V01 tmp1

 spill tree 12
lvaGrabTemp returning 2 (V02 tmp2) called for impSpillStackEnsure.
assign tree 12 to temp 2


               [000019] ------------             *  STMT      void  (IL   ???...  ???)
               [000012] --CXG-------             |  /--*  CALL help ref    HELPER.CORINFO_HELP_READYTORUN_NEWARR_1
               [000010] ------------ arg0        |  |  \--*  CNS_INT   int    16
               [000018] -ACXG-------             \--*  ASG       ref
               [000017] D------N----                \--*  LCL_VAR   ref    V02 tmp2

lvaSetClass: setting class for V02 to (0000000000420038) [System.Private.CoreLib]System.Byte[]

)
    [ 4]  16 (0x010) dup
    [ 5]  17 (0x011) ldtoken
    [ 6]  22 (0x016) call 0A000190
In Compiler::impImportCall: opcode is call, kind=0, callRetType is void, structSize is 0

The last statement is [000019].

The bad example:

    [ 1] 196 (0x0c4) dup
    [ 2] 197 (0x0c5) ldc.i4.s 9
    [ 3] 199 (0x0c7) ldc.i4.s 16
    [ 4] 201 (0x0c9) newarr 01000047
*****************************************SPILL BECAUSE REACHED MAX_TREE_SIZE************************(
 spill tree 273
lvaGrabTemp returning 19 (V19 tmp19) called for impSpillStackEnsure.
assign tree 273 to temp 19


               [000276] ------------             *  STMT      void  (IL   ???...  ???)
               [000273] --CXG-------             |  /--*  CALL help ref    HELPER.CORINFO_HELP_READYTORUN_NEWARR_1
               [000271] ------------ arg0        |  |  \--*  CNS_INT   int    16
               [000275] -ACXG-------             \--*  ASG       ref
               [000274] D------N----                \--*  LCL_VAR   ref    V19 tmp19

lvaSetClass: setting class for V19 to (0000000000420038) [System.Private.CoreLib]System.Byte[]

)
*****************************************SPILL FOR DBG CODE*********************************************(
 spill tree 270
lvaGrabTemp returning 20 (V20 tmp20) called for impSpillStackEnsure.
assign tree 270 to temp 20


               [000280] ------------             *  STMT      void  (IL   ???...  ???)
               [000270] ------------             |  /--*  CNS_INT   int    9
               [000279] -A----------             \--*  ASG       int
               [000278] D------N----                \--*  LCL_VAR   int    V20 tmp20

)
    [ 4] 206 (0x0ce) dup
    [ 5] 207 (0x0cf) ldtoken
    [ 6] 212 (0x0d4) call 0A000190

in the bad example we do not spill [000280] because it is leaf node, that we ignore

if ((opcodeOffs - lastSpillOffs) > MAX_TREE_SIZE && impCanSpillNow(prevOpcode))
{
  impSpillStackEnsure(spillLeaves = false);
}

but then this leave is spilled for dbg code and 000280 becomes the last statement.

I see 3 possible cheap fixes:

  1. do not spill after new_arr, new_obj;
  2. search for the correct stmt in the impInitializeArrayIntrinsic;
  3. swap the order of the spilling because of DBG_CODE and MAX_TREE_SIZE.

I do not like the second variant, because it creates additional checks in the impInitializeArrayIntrinsic.
The third variant is good, possible that there are other intrinsics that will benefit from the right order, but it causes asm diffs in CoreCLR.

So this PR implements the first variant as the cheapest to implement and check.
It is safe not to add dup into this list, because it doesn't add new stmt or locals. It is important not to add it, because we can create a test, that will cause stack overflow with such exclusion.

Fixes coreRT issue with "InitializeArray intrinsic must always be
expanded".
@sandreenko
Copy link
Author

PTAL @dotnet/jit-contrib .

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Someday we should think up a way for this intrinsic to be less fragile.

@sandreenko sandreenko changed the title Fix InitializeArray intrinsic must always be expanded" for CoreRT. Fix InitializeArray intrinsic must always be expanded for CoreRT. Oct 10, 2017
@sandreenko
Copy link
Author

@dotnet-bot test Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness
@dotnet-bot test Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness

@sandreenko sandreenko merged commit 5db8735 into dotnet:master Oct 10, 2017
@sandreenko sandreenko deleted the coreRT branch October 10, 2017 20:37
MichalStrehovsky added a commit to MichalStrehovsky/corert that referenced this pull request Oct 18, 2017
MichalStrehovsky added a commit to dotnet/corert that referenced this pull request Oct 18, 2017
jacano pushed a commit to jacano/corert that referenced this pull request Oct 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants