Skip to content

Enable tests against .NET Core 3.0 SSE and software-only paths #1156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
danmoseley opened this issue Oct 4, 2018 · 6 comments
Closed

Enable tests against .NET Core 3.0 SSE and software-only paths #1156

danmoseley opened this issue Oct 4, 2018 · 6 comments
Assignees

Comments

@danmoseley
Copy link
Member

Seems we have 4 possible code paths in cpumath:

  1. sse.cpp -- used when not on .NET Core 3.0
  2. avxintrinsics.cs -- on .NET Core 3.0 with AVX available
  3. sseintrinsics.cs -- on .NET Core 3.0 with SSE but not AVX
  4. software fallback in cpumathutils.netcoreapp.cs -- on .NET Core 3.0 with no SIMD (eg., on ARM). There seems to be no software fallback when not on .NET Core 3.0.

For tests we have
A) CpuMath.UnitTests.netcoreapp. Executes whichever of (2), (3), (4) applies at run time. Which will almost surely be (2) in all cases.
B) CpuMath.UnitTests.netstandard. Same tests as above, but against (1).
C) Microsoft.ML.CpuMath.PerformanceTests. Perf, not functional, tests for about 20 entry points in (1), (2), and (3) explicitly.

We need to have tests for anything we ship and support, which I think means we have a gap for (3) and (4) ie on older x86 machines and on non x86 machines.

@tannergooding how commonly would the non AVX path (3) be encountered on typical customer x86 machines? Am I correct that (4) is not relevant to x86?

cc @Anipik

@danmoseley
Copy link
Member Author

cc @eerhardt

@eerhardt
Copy link
Member

eerhardt commented Oct 4, 2018

The way to get (3) and (4) exercised is to set environment variables. However, it currently isn't possible (from what I understand) to set environment variables during test execution time. So we will need to add some script that sets the environment variables and then runs the tests.

@danmoseley
Copy link
Member Author

Would using our RemoteExecutorTestBase (with EnvironmentVariables added to RemoteInvokeOptions) do the trick?

@eerhardt
Copy link
Member

eerhardt commented Oct 8, 2018

It's possible that RemoteExecutorTestBase would help here.

In a perfect world, I would still want each test reported as a single test, not all ~72 tests run as one "test". That would mean that each test would need to create its own process, which means the tests are going to take longer than they currently do. We could take that approach to get the coverage now though.

My hope was that we could use a testsettings file for something like this, but it currently appears this isn't possible.

@danmoseley danmoseley changed the title Do we have a complete testing strategy for the math code Enable tests against .NET Core 3.0 SSE and software-only paths Nov 2, 2018
@Anipik
Copy link
Contributor

Anipik commented Jan 18, 2019

@eerhardt @tannergooding @danmosemsft

        [Theory]
        [InlineData("0", null)]
        [InlineData("1", null)]
        [InlineData("0", "COMPlus_EnableAvx")]
        [InlineData("1", "COMPlus_EnableAvx")]
        [InlineData("0", "COMPlus_EnableHWIntrinsic")]
        [InlineData("1", "COMPlus_EnableHWIntrinsic")]
        public void MulElementWiseUTest(string test, string mode)
        {
            RemoteExecutorTestBase.RemoteInvoke((arg1) =>
            {
                float[] src1 = (float[])_testArrays[Int32.Parse(arg1)].Clone();
                float[] src2 = (float[])src1.Clone();
                float[] dst = (float[])src1.Clone();

                // Ensures src1 and src2 are different arrays
                for (int i = 0; i < src2.Length; i++)
                {
                    src2[i] += 1;
                }

                float[] expected = (float[])src1.Clone();

                for (int i = 0; i < expected.Length; i++)
                {
                    expected[i] *= (1 + expected[i]);
                }

                CpuMathUtils.MulElementWise(src1, src2, dst, dst.Length);
                var actual = dst;
                Assert.Equal(expected, actual, _comparer);
                return RemoteExecutorTestBase.SuccessExitCode; 
            }, test, new RemoteInvokeOptions() { mode = mode}).Dispose();

here we are setting the mode environment variable to be 0. This is the blueprint for how all the functions will be looking after the change.

should i go ahead with the change ?

Things which i planning to include
i will most probably add a check in every test to verify which implementation is being used eg,
if avx flag is disabled then Avx.isSupported should be false and Sse.isSupported should be true.

May convert the flag string into enums or some kind of dictionary.

only question i have is regarding running these tests for netcoreapp2.1. If we keep the file(unitTests.cs) the same, each test will be run three times, if we makes the files separate there will be duplication of the code.

if we want to keep the unitests.cs same for netcoreapp2.1 and netcoreapp3.0, we wont be able to able to add checks like if avx.IsSupported.

One solution could be to keep the tests in the same file but put the TheoryData in 2 different files.
Any suggestions or aleterations before i convert everything to new format ?

@Anipik
Copy link
Contributor

Anipik commented Jan 18, 2019

@tannergooding setting COMPlus_EnableAvx and COMPlus_EnableSse to 0 results in Avx.IsSupported and Sse.IsSupported getting false value.

but setting COMPlus_EnableHWIntrinsic to 0 is not setting Avx.IsSupported and Sse.IsSupported to false. am i missing something here ?

@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@danmoseley @eerhardt @Anipik and others