Issue #24343 Vector Ctor using Span #26499

WinCPP · 2018-01-21T17:52:50Z

Fixes #24343.

Depends on dotnet/coreclr#16733 for successful compilation.

Kindly see my comments in the issue #24343. Raising this PR based on my understanding...

@KrzysztofCwalina @terrajobst @karelz @jkotas @mellinoe @ahsonkhan @benaadams @stephentoub Appreciate reviews and feedback.

Thanks,
Mandar

WinCPP · 2018-01-21T17:54:36Z

src/System.Numerics.Vectors/tests/GenericVectorTests.tt

Tried a lot to avoid this change. But don't know how to stop this automatic edit in VS2017 Community edition.

I typically ignore these changes if VS does them automatically. I wouldn't worry about it.

jkotas · 2018-01-21T18:25:37Z

src/System.Numerics.Vectors/ref/System.Numerics.Vectors.cs

@@ -305,6 +305,7 @@ public partial struct Vector<T> : System.IEquatable<System.Numerics.Vector<T>>,
        public Vector(T value) { throw null; }
        public Vector(T[] values) { throw null; }
        public Vector(T[] values, int index) { throw null; }
+        public Vector(System.Span<T> values) { throw null; }


I think that this API can only be in netcoreapp version of this contract. System.Numerics.Vectors is partial OOB. Partial OOB can only move forward with the platform.

hmmm then looks like that is the root cause of the failures that I'm seeing in CI...

I'm not able to figure out how to fix that part... Will wait for guidance.

Between... OOB = "out-of-band"... something similar as 'patch'? Pardon me but...

https://twitter.com/terrajobst/status/915679583873114112 has a good description of the terminology.

The discussion at https://github.com/dotnet/corefx/issues/25182#issuecomment-357006245 is related to this as well.

jkotas · 2018-01-21T18:26:11Z

src/System.Numerics.Vectors/src/System/Numerics/Vector.cs

+        public unsafe Vector(Span<T> values)
+            : this()
+        {
+            if (values == null)


Span can never be null. This check is unnecessary for Span.

WinCPP · 2018-01-21T18:29:51Z

The builds in CI are failing with following error, although it compiled and tested well on my local setup.

System.Numerics.Vectors.cs(304,30): error CS0234: The type or namespace name 'Span<>' does not exist in the namespace 'System' (are you missing an assembly reference?)

I am not too sure what is happening here... My local VS2017 community edition IDE did show a red squiggly for same location (error: System.Span has been deprecated), but I thought it might be because IDE doesn't yet have support for System.Span. Kindly help.

jkotas · 2018-01-21T18:31:12Z

src/System.Numerics.Vectors/src/System/Numerics/Vector.cs

+                throw new IndexOutOfRangeException();
+            }
+
+            if (Vector.IsHardwareAccelerated)


The following 230+ lines should be replaced by a single line that says:

return Unsafe.ReadUnaligned<Vector<T>>(ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(values)));

@jkotas Please refer to this comment in the original issue. I think I heard in the recording that a copy of the contents of the span to the vector will be done... So I based on that understanding, I modelled the entire code on lines of the constructor that accepts T[]... Appreciate your inputs.

Yes, the current Vector code has a lot of complexity because of it targets everything all the way to netstandard10.

If this API is for netcoreapp only, it can be a lot simpler.

jkotas · 2018-01-21T18:37:25Z

src/System.Numerics.Vectors/src/System/Numerics/Vector.cs

+        /// The span must contain at least Vector'T.Count elements.
+        /// </summary>
+        public unsafe Vector(Span<T> values)
+            : this()


Nit: this() should be unnecessary

I tried removing it, but then I got this error... so followed the practise in the Vector.tt file...

System\Numerics\Vector.cs(617,23): error CS0171: Field 'Vector<T>.register' must be fully assigned before control is returned to the caller

jkotas · 2018-01-21T18:40:33Z

cc @eerhardt @ViktorHofer since they are current owners of System.Numerics namespace

KrzysztofCwalina · 2018-01-22T16:26:01Z

How does this compare (perf/codegen wise) to casting the Span the span to Vectors?

WinCPP · 2018-01-22T18:19:28Z

How does this compare (perf/codegen wise) to casting the Span the span to Vectors?

@KrzysztofCwalina I will run the perf tests using the existing solution, but will need guidance on the codegen thing. Haven't done that before.

Between, just bumping up the thread, the CI builds are breaking. I am not sure what to do to make it a partial OOB. Is it that I need to create a separate file for this one API in the 'ref' and 'src' projects such that it will be included in compilation for "netcoreapp" platform only?

jkotas · 2018-01-23T03:58:27Z

src/System.Numerics.Vectors/ref/System.Numerics.Vectors.netcoreapp.cs

+{
+    public partial struct Vector<T> : System.IEquatable<System.Numerics.Vector<T>>, System.IFormattable where T : struct
+    {
+        public Vector(Span<T> values) { throw null; }


Nit: I do not think we split the ref assemblies in multiple files. We tend tend to prefer ifdefs for these.

Done for the ref project...

I've been given the opposite feedback in the past. We split the ref assembly .cs file here:

https://github.com/dotnet/corefx/blob/master/src/System.Net.Sockets/ref/System.Net.Sockets.netcoreapp.cs

A counter example: https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.OpenSsl/ref/System.Security.Cryptography.OpenSsl.cs#L23

I was asked to put them in a separate file here: #25388 (comment)

My (weak) opinion is that we should be consistent throughout our code. In our implementation code we shy away from #if and opt for separate files, so I think we should follow the same pattern in our ref code.

A single System.Numerics.Vectors.cs file in ref project exposes multiple types Vector, Vector2, Vector4, etc. which are defined in different files in the src project. Is it a case that the facade definitions are intended to be kept in single file? Perhaps for full view over what that facade provides? Whereas the actual implementations could be separated out into different cs files? With that logic would it make sense to have single file in the ref project in this and similar cases, unless the types totally unrelated...?

... just asking out of curiosity...

Is it a case that the facade definitions are intended to be kept in single file?

Yes, the reference assembly sources are intentionally kept in single file. They do not follow the conventions used for implementation.

In our implementation code we shy away from #if and opt for separate files

This make sense and works great for Windows/Unix distinction. This distinction is forward looking, it is actively maintained.

It works not so well for the .netcoreapp.cs. The split of netcoreapp is backward looking. We frequently split netcoreapp initially and than abandon the non-netcoreapp flavor. We have many .netcoreapp.cs files in the repo that do make sense anymore because of the whole project is netcoreapp-specific, and there is netcoreapp-specific stuff in the main .cs as well. Hard to understand when you see one of these for the first time.

WinCPP · 2018-01-23T04:36:27Z

@dotnet-bot test Windows x86 Release Build please

WinCPP · 2018-01-23T07:54:26Z

From the CI pipeline report, looks like the Windows x64 Debug Build has been failing on DirectoryServices Tests for some time....

karelz · 2018-01-23T15:57:07Z

Known unrelated issue - see #26531

eerhardt · 2018-01-25T23:13:56Z

src/System.Numerics.Vectors/ref/System.Numerics.Vectors.cs

@@ -305,6 +305,9 @@ public partial struct Vector<T> : System.IEquatable<System.Numerics.Vector<T>>,
        public Vector(T value) { throw null; }
        public Vector(T[] values) { throw null; }
        public Vector(T[] values, int index) { throw null; }
+#if netcoreapp


Typically we put these into separate .netcoreapp.cs files instead of putting #if in the code. See https://github.com/dotnet/corefx/blob/master/src/System.Net.Sockets/ref/System.Net.Sockets.netcoreapp.cs for an example.

eerhardt · 2018-01-25T23:21:36Z

src/System.Numerics.Vectors/src/System/Numerics/Vector.netcoreapp.tt

+
+namespace System.Numerics
+{
+    /* Note: The following patterns are used throughout the code here and are described here


This comment appears to be duplicated with https://github.com/dotnet/corefx/blob/master/src/System.Numerics.Vectors/src/System/Numerics/Vector.tt, can we just reference that comment here? That way the documentation stays in one place.

I have created a T4 function and called it in both places.

eerhardt · 2018-01-25T23:22:15Z

src/System.Numerics.Vectors/src/System/Numerics/Vector.netcoreapp.tt

+    * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
+
+    /// <summary>
+    /// A structure that represents a single Vector. The count of this Vector is fixed but CPU register dependent.


Similar comment here - this comment is duplicated with https://github.com/dotnet/corefx/blob/master/src/System.Numerics.Vectors/src/System/Numerics/Vector.tt#L42

I have created a T4 function and called it in both places.

WinCPP · 2018-01-27T07:52:44Z

@KrzysztofCwalina I have added performance tests. Please review them.

Following is the test result considering the current implementation that involves copying. I will make local changes to implement the way @jkotas suggested and post the data.

Test Name	Metric	Iterations	AVERAGE	STDEV.S	MIN	MAX
System.Numerics.Tests.Constructor.ConstructorBenchmark_Byte	Duration	5	2094.318	5.578	2087.408	2099.439
System.Numerics.Tests.Constructor.ConstructorBenchmark_Double	Duration	33	305.413	0.287	304.774	306.087
System.Numerics.Tests.Constructor.ConstructorBenchmark_Int16	Duration	11	959.458	0.716	958.419	960.875
System.Numerics.Tests.Constructor.ConstructorBenchmark_Int32	Duration	25	401.046	0.989	400.055	402.712
System.Numerics.Tests.Constructor.ConstructorBenchmark_Int64	Duration	33	304.194	0.286	303.624	304.955
System.Numerics.Tests.Constructor.ConstructorBenchmark_SByte	Duration	5	2153.812	0.976	2152.758	2154.790
System.Numerics.Tests.Constructor.ConstructorBenchmark_Single	Duration	25	404.535	9.844	398.968	445.237
System.Numerics.Tests.Constructor.ConstructorBenchmark_UInt16	Duration	11	958.988	0.519	958.459	959.872
System.Numerics.Tests.Constructor.ConstructorBenchmark_UInt32	Duration	25	402.039	0.371	401.379	403.026
System.Numerics.Tests.Constructor.ConstructorBenchmark_UInt64	Duration	33	307.207	8.097	303.811	336.033

WinCPP · 2018-01-27T11:15:00Z

@jkotas I'm not getting to compile the code with following as you mentioned here. Not sure what is going wrong... Help please.

return Unsafe.ReadUnaligned<Vector<T>>(ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(values)));

I get following errors,

"D:\WinCPP\corefx\src\System.Numerics.Vectors\System.Numerics.Vectors.sln" (Rebuild target) (1) ->
"D:\WinCPP\corefx\src\System.Numerics.Vectors\tests\System.Numerics.Vectors.Tests.csproj.metaproj" (Rebuild target) (2) ->
"D:\WinCPP\corefx\src\System.Numerics.Vectors\src\System.Numerics.Vectors.csproj.metaproj" (Rebuild target) (3) ->
"D:\WinCPP\corefx\src\System.Numerics.Vectors\src\System.Numerics.Vectors.csproj" (Rebuild target) (6) ->
(CoreCompile target) ->
  System\Numerics\Vector.netcoreapp.cs(53,20): error CS0103: The name 'Unsafe' does not exist in the current context [D:\WinCPP\corefx\src\System.Numerics.Vectors\src\System.Numer
ics.Vectors.csproj]
  System\Numerics\Vector.netcoreapp.cs(53,56): error CS0103: The name 'Unsafe' does not exist in the current context [D:\WinCPP\corefx\src\System.Numerics.Vectors\src\System.Numer
ics.Vectors.csproj]
  System\Numerics\Vector.netcoreapp.cs(53,79): error CS0103: The name 'MemoryMarshal' does not exist in the current context [D:\WinCPP\corefx\src\System.Numerics.Vectors\src\Syste
m.Numerics.Vectors.csproj]

    0 Warning(s)
    3 Error(s)

Even when I have added the following. I don't get any errors for the following lines though...

using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

jkotas · 2018-01-27T14:42:56Z

It needs to reference CoreLib, similar to https://github.com/dotnet/corefx/pull/25510/files#diff-65f0cef817bd137a2daaea7c28bc5a1aR58 . In addition to this change, it probably needs some change in package authoring to build successfully.

WinCPP · 2018-01-29T20:04:54Z

@jkotas I added the following lines based on the PR that you referred above...

  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Windows_NT-Debug|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Windows_NT-Release|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Unix-Debug|AnyCPU'" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Unix-Release|AnyCPU'" />
  <ItemGroup>
    <ReferenceFromRuntime Include="System.Private.CoreLib" />
  </ItemGroup>

But still no success... I'm not sure how to go about doing this thing. Appreciate inputs...

jkotas · 2018-01-29T20:37:58Z

@eerhardt Could you please take this from here? This is closely related to the other Vector issue that you have stepped up to drive #26266 (comment)

eerhardt · 2018-01-29T20:46:04Z

@WinCPP - can you share all of your changes and explain what doesn't work? It would be easier to see what is wrong that way.

WinCPP · 2018-03-08T17:37:52Z

src/System.Numerics.Vectors/src/System.Numerics.Vectors.csproj

@@ -10,6 +10,7 @@
    <IsTargetingNetCoreApp Condition="'$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'">true</IsTargetingNetCoreApp>
    <IsPartialFacadeAssembly Condition="'$(IsTargetingNetFx)'=='true' OR '$(IsTargetingNetCoreApp)'=='true'">true</IsPartialFacadeAssembly>
    <PackageTargetFramework Condition="'$(TargetGroup)' == 'netstandard1.0'">netstandard1.0;portable-net45+win8+wp8+wpa81</PackageTargetFramework>
+    <DefineConstants Condition="'$(IsTargetingNetCoreApp)' != 'true'">$(DefineConstants);netcoreapp</DefineConstants>


@jkotas @eerhardt this is the latest change to define netcoreapp so that the shared code brought in from dotnet coreclr repo would compile...

This doesn't make sense to me. This is saying "if we aren't targeting netcoreapp, define netcoreapp".

Why wasn't this compiling without this change? What were the errors?

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

No. I was not getting compilation errors. I bringing in changes that would be needed after the merges from coreclr... I meant that.

Between, wouldn't the above line be required to define the preprocessor constant netcoreapp? I thought the TargetGroup is different from the constant used inside the code... that is why I added that line...

TargetGroup is different from the constant used in the code. But what this line is saying is "when IsTargetingNetCoreApp is NOT true, then set the netcoreapp define". That is not correct.

'$(IsTargetingNetCoreApp)' != 'true' => '$(IsTargetingNetCoreApp)' == 'true' would be the right fix. But I don't think this line is necessary at all since we are not compiling the Vector.cs file in this .csproj when targeting netcoreapp.

eerhardt · 2018-03-08T17:51:12Z

src/System.Numerics.Vectors/ref/System.Numerics.Vectors.csproj

@@ -4,6 +4,7 @@
  <PropertyGroup>
    <ProjectGuid>{650277B5-9423-4ACE-BB54-2659995B21C7}</ProjectGuid>
    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netfx' OR '$(TargetGroup)' == 'net46'">true</IsPartialFacadeAssembly>
+	<DefineConstants Condition="'$(TargetGroup)'=='netcoreapp'">$(DefineConstants);netcoreapp</DefineConstants>


Does this need to be '$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'? We want this new API for UAP as well, right?

Yeah you are right. This is the other place in `ref`, didn't see where the comment was. Fixing this.

Note that the ref does not have uapaot-specific build, so you can omit that in the condition for ref.

ok! skipping uapaot in that condition...

eerhardt · 2018-03-08T17:52:14Z

src/System.Numerics.Vectors/tests/GenericVectorTests.cs

@@ -2699,11 +2699,12 @@ private static bool AreSameInfinity(double d1, double d2)
            }
        }

-        internal static T[] GenerateRandomValuesForVector<T>() where T : struct
+        internal static T[] GenerateRandomValuesForVector<T>(int numValues = int.MinValue) where T : struct


(nit) it would probably be better to use int? numValues = null, and then check for null below.

eerhardt · 2018-03-08T17:53:19Z

src/System.Numerics.Vectors/tests/GenericVectorTests.cs

@@ -2699,11 +2699,12 @@ private static bool AreSameInfinity(double d1, double d2)
            }
        }

-        internal static T[] GenerateRandomValuesForVector<T>() where T : struct
+        internal static T[] GenerateRandomValuesForVector<T>(int numValues = int.MinValue) where T : struct


These changes need to be in the .tt file as well, right?

Done. Yes I missed it!!!

eerhardt · 2018-03-08T17:53:55Z

src/System.Numerics.Vectors/tests/System.Numerics.Vectors.Tests.csproj

@@ -35,6 +35,13 @@
      <Link>System\MathF.netstandard.cs</Link>
    </Compile>
  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp'">


Do we want these tests on uap as well?

Just to check... so we don't require uapaot here as well? Please excuse my ignorance :-(

between, will do the same changes to the performance test project as well... they are not there right now.

Right. TargetGroup can only have values listed in Configurations.props.

Thanks @jkotas! Now I understand how that ties up!

WinCPP · 2018-03-08T20:12:13Z

The UWP NETNative x86 Release Build is compiling with TargetGroup as uapaot and then it is failing with this error,

D:\j\workspace\windows-TGrou---c60886e1\Tools\ApiCompat.targets(56,5): error : MembersMustExist : Member 'System.Numerics.Vector<T>..ctor(System.Span<T>)' does not exist in the implementation but it does exist in the contract. [D:\j\workspace\windows-TGrou---c60886e1\src\System.Numerics.Vectors\src\System.Numerics.Vectors.csproj]
D:\j\workspace\windows-TGrou---c60886e1\Tools\ApiCompat.targets(70,5): error : ApiCompat failed for 'D:\j\workspace\windows-TGrou---c60886e1\bin\Windows_NT.AnyCPU.Release\System.Numerics.Vectors\uapaot\System.Numerics.Vectors.dll' [D:\j\workspace\windows-TGrou---c60886e1\src\System.Numerics.Vectors\src\System.Numerics.Vectors.csproj]

But as per the csproj file for 'ref' project, the preprocessor constant netcoreapp won't be defined and hence ideally the Span based constructor should not get included... But contrary seems to be happening... Not sure...

jkotas · 2018-03-08T20:25:37Z

This error should be fixed by adding ApiCompatBaseline file like this one: https://github.com/dotnet/corefx/blob/master/src/System.Threading.ThreadPool/src/ApiCompatBaseline.uapaot.txt

It is a temporary workaround that will go away once the right bits propagate everywhere.

eerhardt · 2018-03-08T20:55:43Z

src/System.Numerics.Vectors/tests/GenericVectorTests.tt

+            T[] arr = new T[count];
+            for (int index = 0; index < count; ++index)
+            {
+                arr[index] = default(T);


C# sets all the elements of a new array to the default value implicitly. We shouldn't need to reset them here. Instead we should just be able to call new T[count] in the calling code and get rid of this method completely.

eerhardt · 2018-03-08T20:57:40Z

src/System.Numerics.Vectors/tests/GenericVectorTests.netcoreapp.cs

+        private void TestArrayIndexBasedConstructorLessElements<T>() where T : struct
+        {
+            T[] values = GenerateRandomValuesForVector<T>(Vector<T>.Count * 2).ToArray();
+            Vector<T> v = new Vector<T>(values, Vector<T>.Count + 1);


I think we could do some verification here to ensure the Vector has the right values in it.

Oh, I see it now.

I think the Assert.Throws should be moved inside this method and only around the new Vector<T> call. Both to reduce duplication and to ensure the exact call is throwing the exception, and not the GenerateRandomValuesForVector method.

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

Done! Here and below, I actually wrote the Span<T> based constructor tests first and then made a copy to make them for T[] based constructor, when I didn't think much :-)

eerhardt · 2018-03-08T20:58:34Z

src/System.Numerics.Vectors/tests/GenericVectorTests.netcoreapp.cs

+        }
+
+        [Fact]
+        public void ArrayBasedConstructorWithLessElements_Byte() => Assert.Throws<IndexOutOfRangeException>(() => TestArrayBasedConstructorWithLessElements<Byte>());


Assert.Throws<IndexOutOfRangeException> could be moved inside of TestArrayBasedConstructorWithLessElements to avoid it being duplicated 10 times here.

eerhardt

Just some nits on the tests that I think could be cleaned up. Other than that, looking good.

ahsonkhan · 2018-03-08T21:40:55Z

src/System.Numerics.Vectors/tests/GenericVectorTests.netcoreapp.cs

+        private void TestSpanBasedConstructorWithLessElements<T>() where T : struct
+        {
+            T[] values = GenerateRandomValuesForVector<T>(Vector<T>.Count - 1).ToArray();
+            Span<T> valueSpan = new Span<T>(values);


nit: use var here since the type can be statically inferred from the right hand side.

ahsonkhan · 2018-03-08T21:41:54Z

src/System.Numerics.Vectors/tests/Performance/Constructor/GenericVectorConstructorTests.cs

+        {
+            Byte[] arrValues = GenerateRandomValuesForVector<Byte>();
+            var spanValues = new Span<Byte>(arrValues);
+            foreach (var iteration in Benchmark.Iterations)


nit: don't use var here.

ahsonkhan · 2018-03-08T21:43:52Z

src/System.Numerics.Vectors/tests/Performance/Constructor/GenericVectorConstructorTests.cs

+                }
+            }
+        }
+


nit: remove extra space

ahsonkhan · 2018-03-08T21:44:49Z

src/System.Numerics.Vectors/tests/Performance/Constructor/GenericVectorConstructorTests.cs

+            {
+                return int.MinValue;
+            }
+            var typeInfo = typeof(T).GetTypeInfo();


nit: don't use var here and below (only use it where the type can be statically inferred from the right hand side).

Actually all this has been copy pasted from the normal test code here

ahsonkhan · 2018-03-08T21:46:36Z

src/System.Numerics.Vectors/tests/Performance/Constructor/GenericVectorConstructorTests.cs

+        [Benchmark(InnerIterationCount = DefaultInnerIterationsCount)]
+        public static void SpanCastBenchmark_Double()
+        {
+            Double[] arrValues = GenerateRandomValuesForVector<Double>();


Please use language keywords instead (here and elsewhere): https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md

We use language keywords instead of BCL types (i.e. int, string, float instead of Int32, String, Single, etc) for both type references as well as method calls (i.e. int.Parse instead of Int32.Parse).

Same as in case of the coreclr PR that we discussed, this comes from existing code for template generation.

In fact, the non-BCL keywords are being emitted by the type.Name property used in the template based code generation., so I think we can hardly do anything with that.

Example of above is that for the unsupported types test, I have added the following here,

Type[] unsupportedTypes = new[] { typeof(Guid), typeof(DateTime), typeof(char) };

Please see the above typeof(char) of char, the language keyword for C# (the language for text template). However, in the places where <#=type.Name#> has been used, it resolves to Char and not char as Type.Name emits the BCL type name. Apparently the entire existing tt code that uses looping and complex logic for code generation depends array of Type (e.g. supporteTypes array). All this would have to be changed to use some soft of simple text insertion (of language keywords) instead of using reflection while processing the various tt files.

ahsonkhan · 2018-03-08T22:09:35Z

src/System.Numerics.Vectors/src/ApiCompatBaseline.uapaot.txt

@@ -0,0 +1 @@
+MembersMustExist : Member 'System.Numerics.Vector<T>..ctor(System.Span<T>)' does not exist in the implementation but it does exist in the contract.


Do we still need to add this exception for uapaot?

Yes, as of a few hours ago this was needed. See #26499 (comment)

I added this as I was getting build errors. further discussion here.

ahsonkhan

LGTM, modulo nits

ahsonkhan · 2018-03-08T22:11:24Z

src/System.Numerics.Vectors/ref/System.Numerics.Vectors.csproj

@@ -3,7 +3,10 @@
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
  <PropertyGroup>
    <ProjectGuid>{650277B5-9423-4ACE-BB54-2659995B21C7}</ProjectGuid>
-    <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netfx' OR '$(TargetGroup)' == 'net46'">true</IsPartialFacadeAssembly>
+    <IsTargetingNetFx Condition="'$(TargetGroup)'=='netfx' OR '$(TargetGroup)'=='net46'">true</IsTargetingNetFx>


Do we need this condition?

Can we just collapse this into the IsPartialFacadeAssembly and use that?

<IsPartialFacadeAssembly Condition="'$(TargetGroup)'=='netfx' OR '$(TargetGroup)'=='net46'">true</IsPartialFacadeAssembly>

IsTargetingNetFx is also used at line 28 below. Hence made the changes to IsPartialFacadeAssembly to consolidate the conditions in one place.

These properties mirror what is in src\System.Numerics.Vectors.csproj

corefx/src/System.Numerics.Vectors/src/System.Numerics.Vectors.csproj

Lines 9 to 12 in 9edb125

<IsTargetingNetFx Condition="'$(TargetGroup)'=='netfx' OR '$(TargetGroup)'=='net46'">true</IsTargetingNetFx>

<IsTargetingNetCoreApp Condition="'$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'">true</IsTargetingNetCoreApp>

<IsPartialFacadeAssembly Condition="'$(IsTargetingNetFx)'=='true' OR '$(IsTargetingNetCoreApp)'=='true'">true</IsPartialFacadeAssembly>

<PackageTargetFramework Condition="'$(TargetGroup)' == 'netstandard1.0'">netstandard1.0;portable-net45+win8+wp8+wpa81</PackageTargetFramework>

I think it is clearer to define them this way, so you can write code later that says:

"If targeting netfx, set this or that"

Instead of code that says:

"If is partial facade assembly, set this or that"

Then you need to constantly look up and say "When is this a partial facade?".

I think it is clearer to define them this way

Good point. I am convinced.

WinCPP · 2018-03-09T03:13:34Z

@dotnet-bot test Windows x64 Debug Build please

eerhardt · 2018-03-09T15:27:37Z

Awesome work, @WinCPP! Thanks for the contribution.

KrzysztofCwalina · 2018-03-09T17:03:48Z

@WinCPP thanks for the contribution!

WinCPP · 2018-03-10T03:41:54Z

Thank you all for a chance to work on this and the guidance! I enjoyed!

WinCPP commented Jan 21, 2018

View reviewed changes

jkotas reviewed Jan 21, 2018

View reviewed changes

karelz added the area-System.Memory label Jan 21, 2018

karelz assigned WinCPP, ahsonkhan and KrzysztofCwalina Jan 21, 2018

ahsonkhan added area-System.Numerics and removed area-System.Memory labels Jan 22, 2018

WinCPP changed the title ~~Issue #24343 Vector Ctor using Span~~ WIP: Issue #24343 Vector Ctor using Span Jan 23, 2018

WinCPP changed the title ~~WIP: Issue #24343 Vector Ctor using Span~~ WIP Issue #24343 Vector Ctor using Span Jan 23, 2018

jkotas reviewed Jan 23, 2018

View reviewed changes

eerhardt reviewed Jan 25, 2018

View reviewed changes

WinCPP force-pushed the issue_24343 branch from 384691d to b652cd3 Compare January 31, 2018 19:57

WinCPP commented Mar 8, 2018

View reviewed changes

eerhardt reviewed Mar 8, 2018

View reviewed changes

eerhardt approved these changes Mar 8, 2018

View reviewed changes

WinCPP force-pushed the issue_24343 branch from 655ba1e to 1da32ab Compare March 8, 2018 21:31

ahsonkhan reviewed Mar 8, 2018

View reviewed changes

ahsonkhan approved these changes Mar 8, 2018

View reviewed changes

ahsonkhan reviewed Mar 8, 2018

View reviewed changes

Issue #24343 Vector Ctor using Span

fff1048

WinCPP force-pushed the issue_24343 branch from 1da32ab to fff1048 Compare March 8, 2018 22:29

eerhardt merged commit 82c6f63 into dotnet:master Mar 9, 2018

karelz added this to the 2.1.0 milestone Mar 10, 2018

WinCPP deleted the issue_24343 branch September 9, 2019 03:41

surban mentioned this pull request Feb 1, 2020

Add support for copying from Vector<T> to Span<T> dotnet/runtime#29929

Closed

		@@ -0,0 +1 @@
		MembersMustExist : Member 'System.Numerics.Vector<T>..ctor(System.Span<T>)' does not exist in the implementation but it does exist in the contract.

	<IsTargetingNetFx Condition="'$(TargetGroup)'=='netfx' OR '$(TargetGroup)'=='net46'">true</IsTargetingNetFx>
	<IsTargetingNetCoreApp Condition="'$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'">true</IsTargetingNetCoreApp>
	<IsPartialFacadeAssembly Condition="'$(IsTargetingNetFx)'=='true' OR '$(IsTargetingNetCoreApp)'=='true'">true</IsPartialFacadeAssembly>
	<PackageTargetFramework Condition="'$(TargetGroup)' == 'netstandard1.0'">netstandard1.0;portable-net45+win8+wp8+wpa81</PackageTargetFramework>

Issue #24343 Vector Ctor using Span #26499

Issue #24343 Vector Ctor using Span #26499

Conversation

WinCPP commented Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 21, 2018 • edited Loading

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

WinCPP commented Jan 21, 2018

jkotas Jan 21, 2018 • edited Loading

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

jkotas commented Jan 21, 2018

KrzysztofCwalina commented Jan 22, 2018

WinCPP commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 25, 2018 • edited Loading

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

WinCPP commented Jan 23, 2018

WinCPP commented Jan 23, 2018

karelz commented Jan 23, 2018

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

WinCPP commented Jan 27, 2018

WinCPP commented Jan 27, 2018

jkotas commented Jan 27, 2018

WinCPP commented Jan 29, 2018

jkotas commented Jan 29, 2018

eerhardt commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinCPP Mar 8, 2018 • edited Loading

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

WinCPP Mar 8, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WinCPP commented Mar 8, 2018

jkotas commented Mar 8, 2018

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

eerhardt 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

WinCPP commented Jan 21, 2018 •

edited

Loading

jkotas Jan 21, 2018 •

edited

Loading

jkotas Jan 21, 2018 •

edited

Loading

jkotas Jan 25, 2018 •

edited

Loading

WinCPP Mar 8, 2018 •

edited

Loading

WinCPP Mar 8, 2018 •

edited

Loading