Port more Vector tests to Vector128/256<T> by fiigii · Pull Request #17737 · dotnet/coreclr

fiigii · 2018-04-23T20:42:18Z

This PR ports the test cases of VectorArray, VectorArgs, and VectorUnused to Vector128/256<T>.
Contribute to https://github.com/dotnet/coreclr/issues/15490

@CarolEidt @tannergooding PTAL

fiigii · 2018-04-24T22:00:06Z

test Windows_NT x64 checked jitstress1
test Windows_NT x64 checked jitstress2
test Windows_NT x86 checked jitstress1
test Windows_NT x86 checked jitstress2
test Ubuntu x64 checked jitstress1
test Ubuntu x64 checked jitstress2

CarolEidt

It would be nice to see more shared code, which I think could be done with one or more helper functions.
Also, since these are closely based on the SIMD tests, it would be good to mention that in a comment, for future reference.

CarolEidt · 2018-04-24T22:32:16Z

+            }
+        }
+
+        static public unsafe int Vector128Array(T deltaValue)


I see that this applies to the existing test that you based this on, but deltaValue doesn't seem to be used. I presume that it was intended to be the value used to initialize the delta vector, but I'm not sure.

CarolEidt · 2018-04-24T22:35:16Z

+            const int vectorSize = 16;
+            int elementCount = vectorSize / elementSize;
+
+            if (typeof(T) == typeof(byte))


Expanding each type makes this much more cumbersome that it needs to be. Is there some reason you didn't make this generic, e.g. using something like the GetValueFromInt<T>() that the SIMD tests use?

fiigii · 2018-04-24T22:48:52Z

@CarolEidt Thank you for the suggestions. Will make these changes.

fiigii · 2018-04-25T22:53:03Z

+        {
+            return Sse2.SetZeroVector128<T>();
+        }
+    }


@CarolEidt @tannergooding Without the fix of #17691, users have to write something like this as the workaround. However, it does not work because

RyuJIT checks the type arguments in the importer, so that the above code cannot be compiled with Debug build (the false-branch of if(typeof(T) == typeof(float)) is not eliminated in the front end).

Assert failure(PID 15536 [0x00003cb0], Thread: 17436 [0x441c]): Assertion failed 'insOfHWIntrinsic(intrinsic, baseType) != INS_invalid' in 'IntelHardwareIntrinsicTest:SetZeroVector128():struct' (IL size 55) File: d:\workspace\coreclr\src\jit\hwintrinsicxarch.cpp Line: 816 Image: D:\workspace\coreclr\bin\tests\Windows_NT.x64.Debug\Tests\Core_Root\CoreRun.exe

the current SSE2 SetZeroVector128 has no HW_Flag_OneTypeGeneric flag so that cannot throw the exception for non-numeric type arguments, which is a critical inconsistent behavior.

#17691 will fix these two issues, please re-consider that PR.

@CarolEidt ping?

I think that these issues can be addressed without #17691, though the usability implications still make an argument.
For now, although it would be more cumbersome, you could put the "else" clause in a separate method, which would not be inlined in Debug, I don't think.
In general, we should ensure that user mis-use doesn't result in an assert in the JIT.

Hmm... OK, will do.

CarolEidt

I think a little more work is needed.

CarolEidt · 2018-04-26T20:44:32Z

+        {
+            return Sse2.SetZeroVector128<T>();
+        }
+    }


I think that these issues can be addressed without #17691, though the usability implications still make an argument.
For now, although it would be more cumbersome, you could put the "else" clause in a separate method, which would not be inlined in Debug, I don't think.
In general, we should ensure that user mis-use doesn't result in an assert in the JIT.

CarolEidt · 2018-04-26T20:48:15Z

+            }
+        }
+
+        static public unsafe int Vector128Array(T deltaValue)


Could you remove this unused value?

Will do. The original S.N.Vector tests also do not use this parameter.

fiigii · 2018-04-26T23:05:40Z

@CarolEidt Addressed all the feedback. PTAL

CarolEidt

LGTM - thanks for making all the changes!

fiigii · 2018-05-01T17:46:22Z

Can we merge this PR?

tannergooding · 2018-05-01T17:49:16Z

+
+        public VectorArg128(float r, float g, float b)
+        {
+            float[] temp = new float[4];


Do the tests need to be a 1-to-1 mapping?

It seems like, in some cases, we could make these more intrinsic oriented (such as just using SetVector128 here, instead of allocating an array)?

Good point, will change.

tannergooding · 2018-05-01T17:51:02Z

+    {
+        int returnVal = Pass;
+
+        if (Sse41.IsSupported)


We should probably support this test on SSE as well. The code for extracting an element isn't too complicated (just a shuffle and convert to scalar).

Thanks, will do

tannergooding

Overall, LGTM. There are some coding-style issues (IMO) but those seem to be taken from the original S.N.Vector code.

fiigii · 2018-05-01T18:21:37Z

I will resolve @tannergooding 's suggestion in the next PR.

dotnet-bot added the 2 - In Progress label Apr 23, 2018

fiigii force-pushed the vectortests branch 2 times, most recently from 3844408 to c80ce9a Compare April 23, 2018 20:46

fiigii closed this Apr 23, 2018

fiigii force-pushed the vectortests branch from c80ce9a to 34c7fe6 Compare April 23, 2018 22:21

dotnet-bot removed the 2 - In Progress label Apr 23, 2018

fiigii reopened this Apr 23, 2018

dotnet-bot added the 2 - In Progress label Apr 23, 2018

fiigii force-pushed the vectortests branch 3 times, most recently from 8134949 to 121909f Compare April 24, 2018 09:05

CarolEidt suggested changes Apr 24, 2018

View reviewed changes

fiigii force-pushed the vectortests branch from 121909f to d7f5650 Compare April 25, 2018 19:14

fiigii commented Apr 25, 2018

View reviewed changes

CarolEidt suggested changes Apr 26, 2018

View reviewed changes

FeiPengIntel added 2 commits April 26, 2018 14:27

Port more Vector tests to Vector128/256

da50c7c

Simplify VectorRet test

9bc22c6

fiigii force-pushed the vectortests branch from ba48f66 to 9bc22c6 Compare April 26, 2018 21:29

CarolEidt approved these changes Apr 27, 2018

View reviewed changes

tannergooding reviewed May 1, 2018

View reviewed changes

tannergooding approved these changes May 1, 2018

View reviewed changes

CarolEidt merged commit 86736f9 into dotnet:master May 1, 2018

fiigii deleted the vectortests branch June 26, 2018 19:13

CarolEidt mentioned this pull request Jan 31, 2020

HW intrinsics shouldn't assert for unsupported dotnet/runtime#10257

Closed

Conversation

fiigii commented Apr 23, 2018

Uh oh!

fiigii commented Apr 24, 2018

Uh oh!

CarolEidt left a comment

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!

fiigii commented Apr 24, 2018

Uh oh!

fiigii Apr 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CarolEidt left a comment

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!

fiigii commented Apr 26, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

fiigii commented May 1, 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!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

fiigii commented May 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fiigii Apr 25, 2018 •

edited

Loading