Implement LoadHigh, LoadLow, and SetScalarVector128 SSE2 HW intrinsics by 4creators · Pull Request #16736 · dotnet/coreclr

4creators · 2018-03-03T17:55:07Z

Addresses code review feedback from #15777

4creators · 2018-03-03T17:58:29Z

Implementation of Compiler::numArgsOfHWIntrinsic was further simplified and due to dropping of default nullptr argument what would prevent inlining fast path part of the function I have refactored it to ensure fast path will be inlined.

fiigii · 2018-03-03T22:02:07Z

 //     number of arguments
 //
-int Compiler::numArgsOfHWIntrinsic(NamedIntrinsic intrinsic, GenTreeHWIntrinsic* node)
+int Compiler::numArgsOfHWIntrinsicSlow(GenTreeHWIntrinsic* node)


I do not think we need two member functions of numArgsOfHWIntrinsic, please combine them to one.

Yes, I thought there was a general consensus that we should have a single method.

CarolEidt · 2018-03-03T22:09:02Z

 //     number of arguments
 //
-int Compiler::numArgsOfHWIntrinsic(NamedIntrinsic intrinsic, GenTreeHWIntrinsic* node)
+int Compiler::numArgsOfHWIntrinsicSlow(GenTreeHWIntrinsic* node)


Yes, I thought there was a general consensus that we should have a single method.

4creators · 2018-03-03T23:35:00Z

@fiigii @CarolEidt

Yes, the consensus was reached that we should have one function but it was based on the assumption that we will have usages where function may be called with node with explicit nullptr value. This design should ensure that in the case the function will be called with default node value C++ compiler would optimize away unreachable slow code.

discussion link: #15777 (comment)

Unfortunately, function in the current code will be called only with node argument which is not nullptr and function due to it's size will not be inlined. To avoid that problem I have used optimization pattern which is always used in .NET Core to enable inlining of function which has small fast code path and which could be good inlining candidate. This pattern leaves fast path in the function which is inlinable and moves large, slow path to subroutine which is called rarely. This pattern is used throughout .NET Core managed and native parts and was discussed in a couple of .NET Core team blog posts.

numArgsOfHWIntrinsic function is called around 3 times for every compiled HW intrinsic and difference between two functions and one function in number of CPU cycles will be roughly 5 cycles for current implementation to 130 - 140 cycles for single function implementation (I assume context switch for every call costs 120-130 CPU cycles. In total we would get ~390 - 420 CPU cycles for each compiled intrinsic with your proposal while we could keep it down to ~15 CPU cycles for majority of instructions what is 2 600 % faster.

The last arguments in favor of proposed design are:

there is general problem with performance of code generation in .NET Core which takes roughly 15% of compilation time
numArgsOfHWIntrinsicSlow can be renamed and hidden as private subroutin

Finally I have introduced signature change by removing NamedIntrinsic intrinsicID argument as this information is passed to function with node. This eliminates the need for assertion assert(node->gtHWIntrinsicId == intrinsic).

4creators · 2018-03-03T23:45:27Z

On Ubuntu we are hitting failures due to renamed System.Runtime.Intrinsics assembly:

Unhandled Exception: System.IO.FileNotFoundException: Could not load file or assembly 'System.Runtime.Intrinsics, Version=4.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. The system cannot find the file specified.

4creators · 2018-03-03T23:46:06Z

Windows job timed out:

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x86_checked_windows_nt_jitx86hwintrinsicnosimd_prtest/164/

fiigii · 2018-03-04T00:31:00Z

Yes, the consensus was reached that we should have one function but it was based on the assumption that we will have usages where function may be called with node with explicit nullptr value. This design should ensure that in the case the function will be called with default node value C++ compiler would optimize away unreachable slow code.

Not really, our consensus is that the function always needs the node as passed argument.

due to it's size will not be inlined

current implementation to 130 - 140 cycles for single function implementation (I assume context switch for every call costs 120-130 CPU cycles. In total we would get ~390 - 420 CPU cycles for each compiled intrinsic with your proposal while we could keep it down to ~15 CPU cycles for majority of instructions what is 2 600 % faster

Interesting, how did you get the data?

numArgsOfHWIntrinsicSlow can be renamed and hidden as private subroutin

I am not a fan of early optimization, but if you can make sure that the function cannot be in-lined in the release build, this approach is reasonable to me. But the slower path can be a static local function instead of private member.

4creators · 2018-03-04T04:36:23Z

test Windows_NT x64 Checked jitx86hwintrinsicnosimd

4creators · 2018-03-04T04:37:28Z

test Windows_NT x86 Checked jitx86hwintrinsicnosimd

tannergooding · 2018-03-04T05:27:03Z

I agree with @fiigii, if we have some actual data indicating this is a good perf scenario, we should make this change. However, the slow path should be a static local function, rather than a member of Compiler.

4creators · 2018-03-04T17:08:24Z

Reverted to single function due to problems with getting static int numArgsOfHWIntrinsic inlined after splitting into two functions. Opened #16744 to track this work item.

@tannergooding @CarolEidt Could you assign it to me?

4creators · 2018-03-04T17:13:33Z

The problem we are hitting now is that numArgsOfHWIntrinsic is never inlined while it was inlined before refactoring:

Disassembly of release build of clrjit.dll CodeGen::genHWIntrinsic

.text:0000000180129C88 CodeGen::genHWIntrinsic proc near 

.text:0000000180129C88 var_B0          = dword ptr -0B0h
.text:0000000180129C88 var_AC          = dword ptr -0ACh
.text:0000000180129C88 var_A8          = qword ptr -0A8h
.text:0000000180129C88 var_A0          = dword ptr -0A0h
.text:0000000180129C88 var_98          = qword ptr -98h
.text:0000000180129C88 var_90          = xmmword ptr -90h
.text:0000000180129C88 var_80          = xmmword ptr -80h
.text:0000000180129C88 var_70          = xmmword ptr -70h
.text:0000000180129C88 var_60          = xmmword ptr -60h
.text:0000000180129C88 var_50          = xmmword ptr -50h
.text:0000000180129C88 var_40          = xmmword ptr -40h
.text:0000000180129C88 var_s18         = dword ptr  18h
.text:0000000180129C88 var_s20         = dword ptr  20h
.text:0000000180129C88 var_s28         = dword ptr  28h
.text:0000000180129C88 arg_0           = qword ptr  40h
.text:0000000180129C88
.text:0000000180129C88                 mov     [rsp-38h+arg_0], rbx
.text:0000000180129C8D                 push    rbp
.text:0000000180129C8E                 push    rsi
.text:0000000180129C8F                 push    rdi
.text:0000000180129C90                 push    r12
.text:0000000180129C92                 push    r13
.text:0000000180129C94                 push    r14
.text:0000000180129C96                 push    r15
.text:0000000180129C98                 lea     rbp, [rsp-27h]
.text:0000000180129C9D                 sub     rsp, 0C0h
.text:0000000180129CA4                 mov     r12d, [rdx+48h]
.text:0000000180129CA8                 mov     r14, rcx
.text:0000000180129CAB                 lea     rcx, hwIntrinsicInfoArray
.text:0000000180129CB2                 mov     rsi, rdx
.text:0000000180129CB5                 lea     eax, [r12-6]
.text:0000000180129CBA                 mov     r11d, eax
.text:0000000180129CBD                 lea     rax, [rax+rax*4]
.text:0000000180129CC1                 add     rax, rax
.text:0000000180129CC4                 mov     r10d, [rcx+rax*8+4Ch]
.text:0000000180129CC9                 mov     r8d, [rcx+rax*8+10h]
.text:0000000180129CCE                 mov     r15d, [rcx+rax*8+48h]
.text:0000000180129CD3                 mov     edi, [rcx+rax*8+14h]
.text:0000000180129CD7                 mov     rcx, rdx
.text:0000000180129CDA                 mov     [rbp+57h+var_A0], r10d
.text:0000000180129CDE                 call    Compiler__numArgsOfHWIntrinsic
.text:0000000180129CE3                 lea     ecx, [r15-3]
.text:0000000180129CE7                 mov     r9d, eax
.text:0000000180129CEA                 mov     eax, 4
.text:0000000180129CEF                 test    ecx, 0FFFFFFFAh
.text:0000000180129CF5                 jnz     short loc_180129D00

fiigii · 2018-03-04T17:51:33Z

that numArgsOfHWIntrinsic is never inlined while it was inlined before refactoring:

That's fine, I do not believe that calling numArgsOfHWIntrinsic could be a bottleneck.

fiigii · 2018-03-04T17:57:30Z

As I said, if a helper function can make numArgsOfHWIntrinsic inlined, we can use a static local function.

fiigii · 2018-03-04T17:58:39Z

-HARDWARE_INTRINSIC(SSE2_SetZeroVector128,                            "SetZeroVector128",                                 SSE2,       -1,           16,          0,            {INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_invalid,   INS_xorpd},             HW_Category_Helper,                            HW_Flag_NoRMWSemantics)
+HARDWARE_INTRINSIC(SSE2_SetAllVector128,                             "SetAllVector128",                                  SSE2,       -1,           16,          1,            {INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid},           HW_Category_Helper,                            HW_Flag_NoCodeGen|HW_Flag_NoRMWSemantics)
+HARDWARE_INTRINSIC(SSE2_SetScalarVector128,                          "SetScalarVector128",                               SSE2,       -1,           16,          1,            {INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_invalid,   INS_movsdsse2},         HW_Category_Helper,                            HW_Flag_MultiIns|HW_Flag_NoRMWSemantics)
+HARDWARE_INTRINSIC(SSE2_SetVector128,                                "SetVector128",                                     SSE2,       -1,           16,         -1,            {INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_pxor,      INS_invalid,   INS_xorpd},             HW_Category_Helper,                            HW_Flag_NoFlag)


Please remove the intrinsic definitions from this table, if you do not implement in the PR.

Ooops, will do.

4creators · 2018-03-04T18:43:36Z

@fiigii Addressed code review feedback

fiigii

LGTM. Thanks for the work.

4creators · 2018-03-04T20:37:06Z

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test

4creators · 2018-03-04T23:05:57Z

On Ubuntu there are failures of Windows sub jobs due to:

Test Infrastructure Failure: Insufficient system resources exist to complete the requested service

https://ci.dot.net/job/dotnet_coreclr/job/master/job/checked_windows_nt_prtest/3399/

4creators · 2018-03-05T00:14:58Z

Failing Windows job timed out:

https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x86_checked_windows_nt_jitx86hwintrinsicnoavx2_prtest/149/

4creators · 2018-03-05T00:16:50Z

test Ubuntu x64 Checked jitincompletehwintrinsic
test Ubuntu x64 Checked jitx86hwintrinsicnoavx
test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2

4creators · 2018-03-05T12:47:42Z

Again hitting timeouts after 4 hours runs

4creators · 2018-03-05T12:48:51Z

test Ubuntu x64 Checked jitincompletehwintrinsic
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
test Windows_NT x64 Checked jitx86hwintrinsicnoavx2

tannergooding · 2018-03-05T15:44:32Z

+            assert(baseType == TYP_DOUBLE);
+            assert(op2 == nullptr);
+
+            instruction ins = Compiler::insOfHWIntrinsic(intrinsicID, node->gtSIMDBaseType);


nit: It might be useful to extract this up-top, like we do in genSSEIntrinsic

@tannergooding Checked the pattern and it seems that in Sse instructions are hard coded:

case NI_SSE_SetScalarVector128: { assert(baseType == TYP_FLOAT); assert(op2 == nullptr); if (op1Reg == targetReg) { regNumber tmpReg = node->GetSingleTempReg(); // Ensure we aren't overwriting targetReg assert(tmpReg != targetReg); emit->emitIns_R_R(INS_movaps, emitTypeSize(TYP_SIMD16), tmpReg, op1Reg); op1Reg = tmpReg; } emit->emitIns_SIMD_R_R_R(INS_xorps, emitTypeSize(TYP_SIMD16), targetReg, targetReg, targetReg); emit->emitIns_SIMD_R_R_R(INS_movss, emitTypeSize(TYP_SIMD16), targetReg, targetReg, op1Reg); break; }

I would prefer to not follow that pattern.

Hmmm, seems I was thinking of another method.

I was suggesting that it not be hard-coded, but you move the instruction ins = Compiler::insOfHWIntrinsic line to the top of genSSE2Intrinsic, so it can be more generally shared.

Actually, I moved it inside case switches due to explicit code review feedback from @CarolEidt

Hmmm - I don't recall giving that feedback (though I don't doubt that I did). It makes sense to pull it out in this case, as nearly all the cases depend upon it. If a large number had a one-to-one mapping, it might make sense to push it into the cases that require it.

In reply to: 172245582 [](ancestors = 172245582)

The comment is here: #16262 (comment)

@CarolEidt Pls confirm if I should refactor

Thanks for the pointer. At that time, there weren't that many cases, so it seemed better to push it down into the cases that needed it, but now there are many cases with the majority requiring it, so it would make sense to pull it up. However, I don't think it's critical.

I am keen to get better implementation. May I do it together with next PRs?

I am keen to get better implementation. May I do it together with next PRs?

That would be fine, though we will want to slow the churn in the code as we move closer to release.

tannergooding · 2018-03-05T15:45:01Z

+            {
+                regNumber tmpReg = node->GetSingleTempReg();
+
+                assert(tmpReg != targetReg);


Please ensure you copy over comments as well when copying code (missing // Ensure we aren't overwriting targetReg)

tannergooding · 2018-03-05T15:47:20Z

-// node unless it is nullptr
+// numArgsOfHWIntrinsic: gets the number of arguments for the hardware intrinsic.
+// This attempts to do a table based lookup but will fallback to the number
+// of operands in 'node' if the table entry is - 1.


nit: Remove the space in - 1

tannergooding · 2018-03-05T15:48:14Z

        {
-            numArgs              = 0;
-            GenTreeArgList* list = op1->AsArgList();
+            int             numArgs = 0;


Why redefine numArgs?

Bad merge - will fix

tannergooding

Overall LGTM.

I plan on creating a template for the Load/Store tests shortly, which should help in code coverage.

4creators · 2018-03-05T17:50:37Z

test Windows_NT x64 Checked jitincompletehwintrinsic
test Windows_NT x64 Checked jitx86hwintrinsicnoavx
test Windows_NT x64 Checked jitx86hwintrinsicnoavx2
test Windows_NT x64 Checked jitx86hwintrinsicnosimd
test Windows_NT x64 Checked jitnox86hwintrinsic

test Windows_NT x86 Checked jitincompletehwintrinsic
test Windows_NT x86 Checked jitx86hwintrinsicnoavx
test Windows_NT x86 Checked jitx86hwintrinsicnoavx2
test Windows_NT x86 Checked jitx86hwintrinsicnosimd
test Windows_NT x86 Checked jitnox86hwintrinsic

test Ubuntu x64 Checked jitincompletehwintrinsic
test Ubuntu x64 Checked jitx86hwintrinsicnoavx
test Ubuntu x64 Checked jitx86hwintrinsicnoavx2
test Ubuntu x64 Checked jitx86hwintrinsicnosimd
test Ubuntu x64 Checked jitnox86hwintrinsic

CarolEidt · 2018-03-05T19:04:47Z

 }

+// TODO_XARCH-CQ - refactoring of numArgsOfHWIntrinsic fast path into inlinable
+// function and slow local static function may increase performance significantly


I don't think we need this comment. As far as I know there is no evidence that this has a measurable impact on throughput.

Thanks for review. May I correct it in next PR?

Yes, that's fine.

CarolEidt

4creators force-pushed the sse2part5 branch from 26cf769 to f4f4ae4 Compare March 3, 2018 18:17

fiigii suggested changes Mar 3, 2018

View reviewed changes

CarolEidt suggested changes Mar 3, 2018

View reviewed changes

4creators force-pushed the sse2part5 branch from f4f4ae4 to 43d5cb8 Compare March 4, 2018 17:02

4creators force-pushed the sse2part5 branch from 43d5cb8 to 2427f43 Compare March 4, 2018 17:16

fiigii reviewed Mar 4, 2018

View reviewed changes

4creators force-pushed the sse2part5 branch from 2427f43 to c60e2ff Compare March 4, 2018 18:42

fiigii approved these changes Mar 4, 2018

View reviewed changes

tannergooding reviewed Mar 5, 2018

View reviewed changes

4creators force-pushed the sse2part5 branch 2 times, most recently from a8d4288 to a72b412 Compare March 5, 2018 16:27

tannergooding approved these changes Mar 5, 2018

View reviewed changes

4creators added 2 commits March 5, 2018 17:39

Implement LoadHigh, LoadLow, and SetScalarVector128 SSE2 HW intrinsics

5cd1398

Add LoadHigh, LoadLow, and SetScalarVector128 SSE2 HW intrinsics tests

03a1f26

4creators force-pushed the sse2part5 branch from a72b412 to 03a1f26 Compare March 5, 2018 16:40

4creators mentioned this pull request Mar 5, 2018

Implement SetAllVector128 SSE2 hardware intrinsic #16758

Closed

CarolEidt reviewed Mar 5, 2018

View reviewed changes

CarolEidt approved these changes Mar 5, 2018

View reviewed changes

tannergooding merged commit a13719b into dotnet:master Mar 6, 2018

4creators deleted the sse2part5 branch August 4, 2018 10:19

4creators mentioned this pull request Jan 31, 2020

Consider refactoring of Compiler::numArgsOfHWIntrinsic function to improve performance dotnet/runtime#9874

Closed

Conversation

4creators commented Mar 3, 2018

Uh oh!

4creators commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4creators commented Mar 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

4creators commented Mar 3, 2018

Uh oh!

4creators commented Mar 3, 2018

Uh oh!

fiigii commented Mar 4, 2018

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

tannergooding commented Mar 4, 2018

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

fiigii commented Mar 4, 2018

Uh oh!

fiigii commented Mar 4, 2018

Uh oh!

fiigii Mar 4, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

fiigii left a comment

Choose a reason for hiding this comment

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

4creators commented Mar 4, 2018

Uh oh!

4creators commented Mar 5, 2018

Uh oh!

4creators commented Mar 5, 2018

Uh oh!

4creators commented Mar 5, 2018

Uh oh!

4creators commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

4creators commented Mar 3, 2018 •

edited

Loading

fiigii Mar 4, 2018 •

edited

Loading

4creators Mar 5, 2018 •

edited

Loading