[RyuJIT/ARM32] Enabling fast tail call feature #14056

hseok-oh · 2017-09-19T01:59:15Z

Not use fast tail call when callee use floating point register argument (difficult to calculate stack size)
Not use fast tail call when callee use split struct argument
Fix importer to compare return type when we check tail call
Fix codegen bug: ARM32 not support INS_br (use INS_bx)

hseok-oh · 2017-09-19T02:02:05Z

#13897 needs to merge first. Without that PR, we need more complicate calculations for arm32 in lvaInitTypeRef() to calculate caller's argument stack & register size.

cc/ @dotnet/arm32-contrib

hseok-oh · 2017-09-19T04:35:35Z

related issue: #13828

Now it can handle when

callee don't have struct argument
callee don't have floating point register argument

jashook · 2017-09-19T15:50:13Z

fyi @dotnet/jit-contrib

jkotas · 2017-09-26T02:55:32Z

src/jit/codegencommon.cpp

+            // Fast tail call.
+            // Call target = REG_R12.
+            // Do we need a special encoding for stack walker like rex.w prefix for x64?
+            getEmitter()->emitIns_R_R(INS_mov, emitTypeSize(TYP_I_IMPL), REG_PC, REG_R12);


Would it be better to use bx r12 to execute the tail call?

I agree with you. I'll change it.

BruceForstall · 2017-10-02T22:51:39Z

@hseok-oh What's next for this PR?

hseok-oh · 2017-10-12T07:45:16Z

@BruceForstall l'll make separate PRs to enable fast tail call, and close this PR.

hseok-oh · 2017-10-23T05:22:00Z

@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test

hseok-oh · 2017-10-23T09:33:18Z

@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test

hseok-oh · 2017-10-30T08:50:05Z

@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test

hseok-oh · 2017-11-01T02:01:47Z

@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test

hseok-oh · 2017-11-02T00:42:55Z

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test
@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test
@dotnet-bot test Windows_NT x64 Innerloop Formatting
@dotnet-bot test Ubuntu x64 Innerloop Formatting

BruceForstall · 2017-11-02T23:29:00Z

Looks like there are still issues (from the x86_arm_altjit run): https://ci.dot.net/job/dotnet_coreclr/job/master/job/x86_arm_altjit_checked_windows_nt_prtest/22/

Assertion failed 'argOffsetOut <= argOffsetMax' in 'Test:A(int,struct,struct):struct' (IL size 58)

BruceForstall · 2017-11-02T23:29:11Z

@dotnet-bot test Windows_NT x86_arm_altjit Checked tailcallstress

hseok-oh · 2017-11-08T08:23:47Z

@dotnet/arm32-contrib @BruceForstall @jashook
I think it's time to enable fast tail call on Linux/ARM32. PTAL.

Not use fast tail call when callee use floating point register argument (difficult to calculate stack size)
Not use fast tail call when callee use split struct argument or struct passed by stack
Fix importer to compare return type when we check tail call
Fix codegen bug: ARM32 not support INS_br (use INS_bx)

jashook

Thank you for the work. Please update the comment above fgCanFastTailCall to include what is supported on arm32 and what is not.

Please see https://github.com/dotnet/coreclr/blob/master/src/jit/morph.cpp#L7145 for examples.

Also the change does not make use of the existing logic to count regArgs and then make a decision at the end of the function based on the count of callee vs caller arguments. Instead it relies on returning earlier before hitting that code path. In my opinion this will make this already cumbersome function harder to review/edit in the future as it does not follow entirely the old paradigm.

jashook · 2017-11-13T18:04:24Z

src/jit/morph.cpp

+
+        argAlign = roundUp(argAlign, TARGET_POINTER_SIZE) / TARGET_POINTER_SIZE;
+
+        // We don't care float register because we will not use fast tailcall


It seems like it is worth tracking fastTailCall support for callee functions with floating point. Is there an issue to track adding support for this later?

In addition what is the rational for not implementing it now?

jashook · 2017-11-13T18:04:59Z

src/jit/morph.cpp

+        argAlign = roundUp(argAlign, TARGET_POINTER_SIZE) / TARGET_POINTER_SIZE;
+
+        // We don't care float register because we will not use fast tailcall
+        // for callee method using float register


Nit: Do not track floating point registers for arm32. It is NYI.

jashook · 2017-11-13T18:08:51Z

src/jit/morph.cpp

+
+                    if (size > 1)
+                    {
+                        // hasTwoSlotSizedStruct will determine if the struct value can be passed  multiple slot.


Nit: two spaces in between passed and multiple

jashook · 2017-11-13T18:11:28Z

src/jit/morph.cpp

+#ifdef _TARGET_ARM_
+            if (varTypeIsFloating(argx))
+            {
+                return false;


Please add logging to this return using reportFastTailCall decision.

Also adding some sort of NYI or comment here explaining why this returns false would be nice.

jashook · 2017-11-13T18:20:23Z

src/jit/morph.cpp

+                            // fastTailCall. This is an implementation limitation
+                            // where the callee only is checked for non enregisterable structs.
+                            // It is tracked with https://github.com/dotnet/coreclr/issues/12644.
+                            hasMultiByteStackArgs = true;


This seems to be using hasMultByteStackArgs in a undesired way as this struct can be passed in registers, there are just more callee arguments and we have to spill. Plus, I am not a big fan of the original hasMultiByteStackArgs code path I would like to avoid building on it.

I think the preferred way of dealing with the case of calleeArgRegCount >= MAX_REG_ARG && hasTwoSlotSizedStruct is here: https://github.com/dotnet/coreclr/blob/master/src/jit/morph.cpp#L7524.

jashook · 2017-11-13T18:22:09Z

src/jit/morph.cpp

+            }
+            unsigned size = genTypeStSz(argx->gtType);
+
+            varTypeIsFloating(argx) ? calleeFloatArgRegCount += size : calleeArgRegCount += size;


Seems better to just have this as:

calleeArgRegCount += size

hseok-oh · 2017-11-23T08:48:22Z

@jashook Sorry for late feedback. I'll fix it soon.

hseok-oh · 2017-11-27T13:14:52Z

@jashook I've updated.

Add comment in fgCanFastTailCall: condition to allow fast tail call on ARM
Change return and report position to end of function

PTAL

hseok-oh · 2017-12-03T00:39:13Z

@dotnet-bot test Ubuntu x64 Innerloop Formatting

jashook

Sorry for taking a little to get to this thank you for the changes.

jashook · 2017-12-14T17:47:38Z

@BruceForstall ptal

CarolEidt

I'm not completely familiar with the fast tail call implementation, but the changes look good to me.

jashook · 2017-12-14T18:47:34Z

@dotnet-bot test this please

BruceForstall · 2018-01-03T23:39:44Z

@dotnet-bot test this please

BruceForstall · 2018-01-03T23:46:11Z

@dotnet-bot test Windows_NT arm Cross Checked jitstress1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress2 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked tailcallstress Build and Test
@dotnet-bot test Windows_NT x86_arm_altjit Checked tailcallstress
@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test

BruceForstall · 2018-01-04T02:22:43Z

armlb failure is known.

@dotnet-bot test Windows_NT x86_arm_altjit Checked tailcallstress

BruceForstall · 2018-01-04T19:39:47Z

Looks like a lot of test failures need to be investigated. In a short survey, I saw a lot of access violations in the JitStress runs, and VM asserts in the TailcallStress modes.

BruceForstall

Test failures need to be investigated/fixed.

- Not use fasttailcall when callee use floating point register argument (difficult to calculate stack size) - Not use fasttailcall when callee use split struct argument - Fix importer to compare return type when we check tail call - Fix codegen bug: ARM32 not support INS_br (use INS_bx)

Add comment in fgCanFastTailCall for ARM32

hseok-oh · 2018-01-08T11:40:50Z

@dotnet-bot test Windows_NT arm Cross Checked jitstress1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress2 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked tailcallstress Build and Test
@dotnet-bot test Windows_NT x86_arm_altjit Checked tailcallstress
@dotnet-bot test Windows_NT x86_arm_altjit Checked Build and Test

BruceForstall · 2018-01-08T18:28:56Z

@dotnet-bot test Windows_NT arm Cross Checked jitstress1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked jitstress2 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked tailcallstress Build and Test

BruceForstall · 2018-02-13T00:33:12Z

Waiting until #16039 is resolved before reviewing this again.

BruceForstall · 2018-02-14T21:12:09Z

@hseok-oh Can you fix the conflict so we can trigger retesting?

@alpencolt Are you taking this over?

alpencolt · 2018-02-15T09:04:05Z

@BruceForstall I'll take this one too.

alpencolt · 2018-02-16T12:10:36Z

@BruceForstall I've rebased PR but cannot push to GitHub:

$ git push origin  ryujit/fasttailcall
Username for 'https://github.com': alpencolt
Password for 'https://alpencolt@github.com': 
remote: Permission to dotnet/coreclr.git denied to alpencolt.
fatal: unable to access 'https://github.com/dotnet/coreclr.git/': The requested URL returned error: 403

It looks there is not enough permission or may be I do something wrong?
If I will push to my repo the new PR will will be created. What would you recommend?

BruceForstall · 2018-02-16T17:23:22Z

@alpencolt You should just push to your fork and create a new PR. Then close this one (with a link/reference to your new one).

alpencolt · 2018-02-16T17:37:19Z

#16419

dnfclas added the cla-already-signed label Sep 19, 2017

jashook self-requested a review September 19, 2017 15:47

BruceForstall self-requested a review September 19, 2017 21:04

hseok-oh force-pushed the ryujit/fasttailcall branch from 1b1b4f4 to c587ac6 Compare September 20, 2017 08:39

hseok-oh mentioned this pull request Sep 26, 2017

[RyuJIT/ARM32] Fast tail call: Init LSRA to use R12 to save jump target #14184

Merged

jkotas reviewed Sep 26, 2017

View reviewed changes

jashook added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 27, 2017

hseok-oh mentioned this pull request Oct 12, 2017

[RyuJIT/ARM32] Fast tail call: code generation #14445

Merged

hseok-oh force-pushed the ryujit/fasttailcall branch from c587ac6 to dcd6e2d Compare October 23, 2017 05:19

hseok-oh changed the title ~~[RyuJIT/ARM32] [WIP] [DO NOT MERGE] Enabling fast tail call feature~~ [RyuJIT/ARM32] Enabling fast tail call feature Oct 23, 2017

hseok-oh force-pushed the ryujit/fasttailcall branch from dcd6e2d to 9f23b52 Compare October 23, 2017 09:32

hseok-oh changed the title ~~[RyuJIT/ARM32] Enabling fast tail call feature~~ [RyuJIT/ARM32] [WIP] Enabling fast tail call feature Oct 23, 2017

hseok-oh force-pushed the ryujit/fasttailcall branch from 9f23b52 to 03679e8 Compare October 30, 2017 08:49

hseok-oh force-pushed the ryujit/fasttailcall branch from 03679e8 to c7cd0b4 Compare November 1, 2017 02:00

hseok-oh changed the title ~~[RyuJIT/ARM32] [WIP] Enabling fast tail call feature~~ [RyuJIT/ARM32] Enabling fast tail call feature Nov 2, 2017

BruceForstall removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 2, 2017

hseok-oh force-pushed the ryujit/fasttailcall branch 2 times, most recently from 9d05aa1 to 2c62e49 Compare November 3, 2017 08:47

jashook suggested changes Nov 13, 2017

View reviewed changes

jashook approved these changes Dec 14, 2017

View reviewed changes

CarolEidt approved these changes Dec 14, 2017

View reviewed changes

jkotas added the area-CodeGen label Dec 28, 2017

BruceForstall approved these changes Jan 4, 2018

View reviewed changes

BruceForstall suggested changes Jan 4, 2018

View reviewed changes

hseok-oh added 2 commits January 8, 2018 20:33

Add comment in fgCanFastTailCall

afcdc72

Add comment in fgCanFastTailCall for ARM32

hseok-oh force-pushed the ryujit/fasttailcall branch from 5d6ebf5 to afcdc72 Compare January 8, 2018 11:35

BruceForstall assigned alpencolt Feb 15, 2018

alpencolt mentioned this pull request Feb 16, 2018

[RyuJIT/ARM32] Enabling fast tail call feature #16419

Closed

jkotas closed this Feb 17, 2018


		argAlign = roundUp(argAlign, TARGET_POINTER_SIZE) / TARGET_POINTER_SIZE;

		// We don't care float register because we will not use fast tailcall

[RyuJIT/ARM32] Enabling fast tail call feature #14056

[RyuJIT/ARM32] Enabling fast tail call feature #14056

Conversation

hseok-oh commented Sep 19, 2017 • edited Loading

hseok-oh commented Sep 19, 2017 • edited Loading

hseok-oh commented Sep 19, 2017

jashook commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall commented Oct 2, 2017

hseok-oh commented Oct 12, 2017

hseok-oh commented Oct 23, 2017

hseok-oh commented Oct 23, 2017

hseok-oh commented Oct 30, 2017

hseok-oh commented Nov 1, 2017

hseok-oh commented Nov 2, 2017

BruceForstall commented Nov 2, 2017

BruceForstall commented Nov 2, 2017

hseok-oh commented Nov 8, 2017

jashook left a 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

hseok-oh commented Nov 23, 2017

hseok-oh commented Nov 27, 2017

hseok-oh commented Dec 3, 2017

jashook left a comment

Choose a reason for hiding this comment

jashook commented Dec 14, 2017

CarolEidt left a comment

Choose a reason for hiding this comment

jashook commented Dec 14, 2017 • edited Loading

BruceForstall commented Jan 3, 2018

BruceForstall commented Jan 3, 2018

BruceForstall commented Jan 4, 2018

BruceForstall commented Jan 4, 2018

BruceForstall left a comment

Choose a reason for hiding this comment

hseok-oh commented Jan 8, 2018

BruceForstall commented Jan 8, 2018

BruceForstall commented Feb 13, 2018

BruceForstall commented Feb 14, 2018

alpencolt commented Feb 15, 2018

alpencolt commented Feb 16, 2018

BruceForstall commented Feb 16, 2018

alpencolt commented Feb 16, 2018

hseok-oh commented Sep 19, 2017 •

edited

Loading

hseok-oh commented Sep 19, 2017 •

edited

Loading

jashook commented Dec 14, 2017 •

edited

Loading