[ARM32/RyuJIT] Enable passing struct argument that use stack only by hseok-oh · Pull Request #11541 · dotnet/coreclr

hseok-oh · 2017-05-12T05:08:08Z

Enable passing struct argument when it uses stack only.
Cannot pass splitted struct argument that uses stack and register(s) yet.

related issue: #10722

Enable passing struct argument when it uses stack only. Cannot pass splitted struct argument that uses stack and register(s) yet.

hseok-oh · 2017-05-12T05:13:13Z

cc/ @dotnet/arm32-contrib

Example

using System;
using System.Runtime.CompilerServices;

public struct IntStruct
{   
    public int num1;
    public int num2;
    public int num3;
    public int num4;
    public int num5;
}

public class BringUpTest
{
    const int Pass = 100;
    const int Fail = -1;

    [MethodImplAttribute(MethodImplOptions.NoInlining)]
    public static int StructAdd(int a, int b, int c, int d, IntStruct rp)
    {   
        return a + b+ c + d + rp.num1 + rp.num2 + rp.num3 + rp.num4 + rp.num5;
    }

    public static int Main()
    {   
        IntStruct a = new IntStruct();
        
        a.num1 = 3;
        a.num2 = 2;
        a.num3 = 4;
        a.num4 = 1;
        a.num5 = 6;

        int y = StructAdd(1, 2, 3, 4, a);
        if (y == 16) return Pass;
        else return Fail;
    }
}

Before

Assert failure(PID 11626 [0x00002d6a], Thread: 11626 [0x2d6a]): Assertion failed 'NYI: Unimplemented node type obj' in 'BringUpTest:Main():int' (IL size 11894)

    File: /home/sjsujinkim/works/dotnet/coreclr/src/jit/lsraarm.cpp Line: 702
    Image: /nfs/rbp3/test/coreoverlay_checked/corerun

Aborted

After

generated code to call StructAdd

Generating: N043 (  3,  2) [000040] -------N-----       t40 =    &lclVar   byref  V00 loc0         u:8 (last use) REG NA
                                                              /--*  t40    byref
Generating: N045 (  9,  7) [000043] -------------       t43 = *  obj(20)   struct REG NA $101
                                                              /--*  t43    struct  
Generating: N047 (???,???) [000080] -------------             *  putarg_stk [+0x00] struct REG NA  
IN0011:             ldr     r0, [sp+0x1c]       // [V00 loc0]
IN0012:             str     r0, [sp]    // [V01 OutArgs]
IN0013:             ldr     r0, [sp+0x1c]       // [V00 loc0]    
IN0014:             str     r0, [sp+0x04]       // [V01 OutArgs+0x04]
IN0015:             ldr     r0, [sp+0x1c]       // [V00 loc0] 
IN0016:             str     r0, [sp+0x08]       // [V01 OutArgs+0x08]
IN0017:             ldr     r0, [sp+0x1c]       // [V00 loc0] 
IN0018:             str     r0, [sp+0x0c]       // [V01 OutArgs+0x0c]
IN0019:             ldr     r0, [sp+0x1c]       // [V00 loc0]
IN001a:             str     r0, [sp+0x10]       // [V01 OutArgs+0x10]
Generating: N049 (  1,  1) [000036] -------------       t36 =    const     int    1 REG r0 $85
IN001b:             movs    r0, 1
                                                              /--*  t36    int
Generating: N051 (???,???) [000081] -------------       t81 = *  putarg_reg int    REG r0
Generating: N053 (  1,  1) [000037] -------------       t37 =    const     int    2 REG r1 $83
IN001c:             movs    r1, 2
                                                              /--*  t37    int
Generating: N055 (???,???) [000082] -------------       t82 = *  putarg_reg int    REG r1
Generating: N057 (  1,  1) [000038] -------------       t38 =    const     int    3 REG r2 $82
IN001d:             movs    r2, 3
                                                              /--*  t38    int    
Generating: N059 (???,???) [000083] -------------       t83 = *  putarg_reg int    REG r2
Generating: N061 (  1,  1) [000039] -------------       t39 =    const     int    4 REG r3 $84
IN001e:             movs    r3, 4
                                                              /--*  t39    int    
Generating: N063 (???,???) [000084] -------------       t84 = *  putarg_reg int    REG r3
Generating: N065 (  3,  7) [000085] -------------       t85 =    const(h)  int    0x74F57A14 ftn REG lr
IN001f:             movw    lr, 0x7a14
IN0020:             movt    lr, 0x74f5
                                                              /--*  t85    int
Generating: N067 (  6,  9) [000086] -------------       t86 = *  indir     int    REG lr
IN0021:             ldr     lr, [lr]
                                                              /--*  t81    int    arg0 in r0
                                                              +--*  t82    int    arg1 in r1
                                                              +--*  t83    int    arg2 in r2
                                                              +--*  t84    int    arg3 in r3
                                                              +--*  t86    int    control expr
Generating: N069 ( 32, 26) [000041] --C-G--------       t41 = *  call      int    BringUpTest.StructAdd $204
Call: GCvars=00000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}
IN0022:             blx     lr          // BringUpTest:StructAdd(int,int,int,int,struct):int

Fix formatting

Fix formatting error

BruceForstall · 2017-05-12T17:18:34Z

+            {
+                return;
+            }
+#endif // _TARGET_ARM_


This doesn't look necessary (or desirable).

However, it looks like there is an existing bug where nextType/nextAttr below is accessed even if remainingSize==0. That could be fixed by pushing these declarations within the first if, and then deleting the

nextType = compiler->getJitGCType(gcPtrs[nextIndex]); nextAttr = emitTypeSize(nextType);

at the end of the first if.

BruceForstall · 2017-05-12T17:21:28Z

-            unsigned gcPtrCount;                     // The count of GC pointers in the struct
+            BYTE* gcPtrs                        = nullptr;
+            BYTE  gcPtrArray[MAX_ARG_REG_COUNT] = {}; // TYPE_GC_NONE = 0
+


For simplicity, you could just always initialize gcPtrs to gcPtrArray:

BYTE gcPtrArray[MAX_ARG_REG_COUNT] = {}; // TYPE_GC_NONE = 0 BYTE* gcPtrs = gcPtrArray;

BruceForstall · 2017-05-12T17:25:59Z

There is code in Lowering::TreeNodeInfoInitPutArgStk() that is specific to ARM64 that needs adjusting:

            // We could use a ldp/stp sequence so we need two internal registers
            argNode->gtLsraInfo.internalIntCount = 2;

For arm32, you only need one internal register.

BruceForstall · 2017-05-12T17:28:07Z

+                gcPtrs     = treeNode->gtGcPtrs;
+                gcPtrCount = treeNode->gtNumSlots;
+#endif // _TARGET_ARM_
            }


Can you explain why this is ARM32-specific? Why does ARM64 do something different here?

treeNode->gtGcPtrs and treeNode->gtNumSlots can be used when FEATURE_PUT_STRUCT_ARG_STK is set, but ARM64 is not use this feature.

- Remove redundant GC type assignment in genPutArgStk - Fix internal register count for ARM32: 2 -> 1

Fix formatting

hseok-oh · 2017-05-16T04:32:12Z

@BruceForstall Fixed it by applying your review. PTAL

hqueue · 2017-05-18T09:50:11Z

+#else  // _TARGET_ARM_
+            // For a >= 4 byte structSize we will generate a ldr and str instruction each loop
+            //             ldr     r2, [r0]
+            //             str     r2, [sp, #16]


How about str r2, [sp, #4] ?
Because we are going to increase offset by TARGET_POINTER_SIZE for ARM32 here.

Or does the offset start from 16 and increase by 4 ?

I think it's not important what the offset value is. It is just example. This offset value isn't offset of this struct argument. It means stack offset in all of arguments for method.

hseok-oh · 2017-05-18T10:09:21Z

Please merge this after merging #11709.
Whitout #11709, it can make new runtime error when remorphing phase.

hseok-oh · 2017-05-19T02:27:16Z

Please merge this after merging #11709.
Whitout #11709, it can make new runtime error when remorphing phase.
This PR is independent with #11709. It was my mistake.

BruceForstall · 2017-05-19T21:40:27Z

@dotnet-bot test this please

BruceForstall · 2017-05-19T21:40:45Z

@dotnet-bot test Windows_NT arm64 Checked

BruceForstall · 2017-05-22T05:11:18Z

@dotnet-bot test Tizen armel Cross Debug Build
@dotnet-bot test Windows_NT arm64 Checked

[ARM32/RyuJIT] Enable passing struct argument that use stack only

fb23014

Enable passing struct argument when it uses stack only. Cannot pass splitted struct argument that uses stack and register(s) yet.

dnfclas added the cla-already-signed label May 12, 2017

hseok-oh added 2 commits May 12, 2017 14:23

Fix formatting

38a2b10

Fix formatting

Fix formatting

ec66101

Fix formatting error

BruceForstall self-requested a review May 12, 2017 06:46

BruceForstall reviewed May 12, 2017

View reviewed changes

hseok-oh added 2 commits May 15, 2017 10:58

Remove redundant GC type assignment and fix internal register count

b9dd9b8

- Remove redundant GC type assignment in genPutArgStk - Fix internal register count for ARM32: 2 -> 1

Fix formatting

9221dc6

Fix formatting

hqueue reviewed May 18, 2017

View reviewed changes

BruceForstall merged commit 4349824 into dotnet:master May 22, 2017

hseok-oh deleted the ryujit/struct_arg_stack branch May 31, 2017 04:31

karelz modified the milestone: 2.1.0 Aug 28, 2017

Conversation

hseok-oh commented May 12, 2017

Uh oh!

hseok-oh commented May 12, 2017

Example

Before

After

Uh oh!

BruceForstall May 12, 2017

Choose a reason for hiding this comment

Uh oh!

BruceForstall May 12, 2017

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented May 12, 2017

Uh oh!

BruceForstall May 12, 2017

Choose a reason for hiding this comment

Uh oh!

hseok-oh May 15, 2017

Choose a reason for hiding this comment

Uh oh!

hseok-oh commented May 16, 2017

Uh oh!

hqueue May 18, 2017

Choose a reason for hiding this comment

Uh oh!

hqueue May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hseok-oh May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hseok-oh commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hseok-oh commented May 19, 2017

Uh oh!

BruceForstall commented May 19, 2017

Uh oh!

BruceForstall commented May 19, 2017

Uh oh!

BruceForstall commented May 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hqueue May 18, 2017 •

edited

Loading

hseok-oh May 18, 2017 •

edited

Loading

hseok-oh commented May 18, 2017 •

edited

Loading