CoreFX #24343 Vector Ctor using Span #16733

WinCPP · 2018-03-03T13:32:18Z

This PR is for new constructor for System.Numercis.Vector in library System.Private.CoreLib that accepts a Span for the issue dotnet/corefx#24343.

dotnet/corefx#26499 is dependent on merging of this PR for successful compilation.

@eerhardt Kindly review.

jkotas · 2018-03-03T15:23:21Z

src/mscorlib/shared/System/Numerics/Vector.cs

@@ -2,9 +2,11 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using Internal.Runtime.CompilerServices;


This needs to be under ifdef. Otherwise, it won't build once mirrored to CoreFX used to build the portable Vector version.

Mirroring as in, is there going to be an attempt later sometime to single-instance the copies of shared code base between CoreClr and CoreFx repos? In that case do you want me to add #ifdef netcoreapp in here with appropriate definition of netcoreapp in the csproj?

This single instancing is in place already. Once this PR is merged, there will be PR with your change created in corefx automatically: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/README.md

We use #if !netstandard in other similar places: https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs#L11 . I would try to stick with that unless if does not work for some reason.

In CoreFx, we include this file when building for netfx and netstandard. So I'm guessing !netstandard isn't going to be enough.

Also - the whole new constructor should probably be under the ifdef, so it doesn't exist in the netfx and netstandard versions.

@eerhardt I again went through the csproj for CoreFx project and in there Vector.cs is included if not targetting netcoreapp. I am thoroughly confused when it comes to nomenclature such as netstandard vs netfx, netcoreapp, etc. So do you mean to say that I should put the using statement and entire new constructor under #if (!netfx && !netstandard). And then the two (corefx csproj condition vs the conditional compilation in this repo) appear to conflict.

I am not able to understand how this #if compares to the conditional compilation in the CoreFx csproj. Hence please help with what I should actually use.

I am thoroughly confused when it comes to nomenclature such as netstandard vs netfx, netcoreapp, etc.

netcoreapp means .NET Core.
netfx means .NET Framework.

Both the .NET Core and .NET Framework are concrete implementations of a runtime that executes .NET code. This means you can actually run code on them. The .NET Framework ships as part of Windows and only runs on Windows. .NET Core runs on Windows, Linux and OSX.

You can think of netstandard as an abstraction a developer can target that will allow their code to run on any concrete runtime: .NET Core, .NET Framework, Mono, etc. netstandard isn't a runtime itself, but it is a contract that the concrete runtimes implement.

So do you mean to say that I should put the using statement and entire new constructor under #if (!netfx && !netstandard).

I think it should actually be under #if CORECLR (which is the DefineConstant used by coreclr to mean "this is netcoreapp").

When building this code in dotnet/corefx for netfx and netstandard that DefineConstant won't be defined, so this code won't be compiled.

When building this code in dotnet/corefx for netfx and netstandard that DefineConstant won't be defined, so this code won't be compiled.

Thanks @eerhardt for the explanation. Actually what confused is me that if netstandard is the standard and netfx is implementation then why was I supposed to use both in that #if. And just when I thought I had reconfirmed my understanding by reading your above reply, I again got stuck at the last line in which you have mentioned netfx and netstandard in same line :-) . I thought netstandard includes netfx (standard and implementation relationship). So did you want to mention netcoreapp instead of netstandard ...? In other words is it necessary to explicitly say netfx when I say netstandard... Hope I am not creating more confusion.

As for the PR, I am replacing it iwth CORECLR as you have mentioned.

iwth CORECLR as you have mentioned.

That will not compile on CoreRT and ProjectN. It would need to be more combined with more conditions there. I think #if netcoreapp and defining netcoreapp when building S.P.CoreLib as you have proposed originally is probably the best choice.

@jkotas Thank you. I have pushed a commit that defines netcoreapp and uses in the .cs file.

jkotas · 2018-03-03T15:31:19Z

src/mscorlib/shared/System/Numerics/Vector.cs

+        [Intrinsic]
+        public unsafe Vector(Span<T> values)
+            : this()
+        {


This needs bounds check to verify that there is enough elements in the Span, similar to constructors that take arrays.

(Add test for it to your CoreFX PR too.)

Working on this. I plan to model the new exception on lines of the ask in issue dotnet/corefx#25344. Whatever is done here will have to be replicated in fix for dotnet/corefx#25344. (Would like to work on that after this.)

jkotas · 2018-03-03T15:38:34Z

src/mscorlib/shared/System/Numerics/Vector.cs

+        public unsafe Vector(Span<T> values)
+            : this()
+        {
+            this = Unsafe.ReadUnaligned<Vector<T>>(ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(values)));


Do we need to validate that T is valid vector type here, similar how array constructors validate it?

Otherwise, I will be able to do things like:

Span<string> s = new string[] { "a", ... }; Vector<string> v = new Vector<string>(s);

that is going bad things to happen.

The Vector definition is as follows. So will that exclude the above cases...?

public struct Vector<T> : IEquatable<Vector<T>>, IFormattable where T : struct

You are right that will exclude my example with string, but it won't exclude other structs, e.g.:

Span<Guid> s = Vector<Guid> v = new Vector<Guid>(s);

Thanks. Earlier I missed the importance of supportedTypes defined in the GenerationConfig.ttinclude file; which is used in the T[] constructor already. I will use the same construct here.

jkotas · 2018-03-03T15:41:15Z

BTW: Many existing methods can be optimized using Unsafe in similar way: InitializeCount can just call Unsafe.SizeOf<T>, Vector(T[] values, int index), Vector(void* dataPointer, int offset) can just call this Span-based constructor, CopyTo can just do Unsafe.WriteUnaligned, ...

WinCPP · 2018-03-03T18:23:34Z

BTW: Many existing methods can be optimized using Unsafe in similar way: InitializeCount can just call Unsafe.SizeOf, Vector(T[] values, int index), Vector(void* dataPointer, int offset) can just call this Span-based constructor, CopyTo can just do Unsafe.WriteUnaligned, ...

Agreed, So should I take this up as part of this PR or we want to open another issue? I would like to work on that.

jkotas · 2018-03-03T20:58:17Z

should I take this up as part of this PR

Up to you. Separate PR may be better (smaller PRs are easier to get through).

fiigii · 2018-03-04T18:29:52Z

src/mscorlib/shared/System/Numerics/Vector.cs

+        /// <summary>
+        /// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.
+        /// </summary>
+        [Intrinsic]


Why do we need this attribute? Vector<T> intrinsics are not recognized by the new-style intrinsic system.

BTW, this new constructor is just a software implementation rather than intrinsic.

eerhardt · 2018-03-05T16:30:45Z

src/mscorlib/shared/System/Numerics/Vector.cs

+            {
+                if (values.Length < Count)
+                {
+                    throw new IndexOutOfRangeException(SR.Format(SR.Arg_InsufficientNumberOfElements, Vector<T>.Count, nameof(values)));


It may help users if this exception told them values.Length as well.

I had thought about this and was ok for this instance. But there is another instance for T[] based constructor which I reintroduced just now as per review this comment by @4creators. In that case, the statement becomes complex when an array and an index is mentioned. In that case it is not simply the number of elements in value but the relative number of elements in value with respect to the index.

I was trying to have consistency in the exception being thrown across the three usages and with few other exceptions in the resources file e.g.

Arg_ElementsInSourceIsGreaterThanDestination - Number of elements in source vector is greater than the destination array

Argument_InsufficientSpaceToCopyCollection - The specified space is not sufficient to copy the elements from this Collection.

And anyways, the user would know the number of elements that (s)he is sending...

eerhardt · 2018-03-05T16:32:43Z

src/mscorlib/shared/System/Numerics/Vector.cs

+        /// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.
+        /// </summary>
+        [Intrinsic]
+        public unsafe Vector(Span<T> values)


Is this method still unsafe?

Removing it. It is left over from the earlier implementation in CoreFx code base that was on lines of constructor taking arrays, i.e. one which used pointers.

eerhardt · 2018-03-05T16:34:08Z

src/mscorlib/shared/System/Numerics/Vector.cs

+        /// <summary>
+        /// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.
+        /// </summary>
+        [Intrinsic]


I'm not sure the [Intrinsic] attribute is necessary/correct here. I thought it only applied to members that the JIT needed to know about. I don't think we need JIT interception here, do we?

This method should have performance on par with the method that takes arrays. If the one that takes array needs to be treated as JIT intrinsic to get good performance, the one that takes Span needs to be treated as JIT intrinsic as well; and vice versa.

Oops. I was too fast to remove the attribute and push a commit :-) Sorry. I think I will wait till the discussions are over.

Apart from this discussion there was another similar review comment by @fiigii at this place. I'm not sure whether or not to retain the [Intrinsic] attribute. Appreciate inputs please.

eerhardt · 2018-03-05T16:37:19Z

src/mscorlib/shared/System/Numerics/GenerationConfig.ttinclude

+    {
+        StringBuilder sbuilder = new StringBuilder();
+        var arrAllTypes = allTypes.ToArray();


Don't need to call ToArray here. Instead, you can just enumerate allTypes and keep track if this is the first time through the loop.

bool firstTime = true; foreach (Type type in allTypes) { if (firstTime) { sbuilder.Append("if (").Append(MakeTypeComparisonCondition(type)); firstTime = false; } else { sbuilder.AppendLine().Append(" || ").Append(MakeTypeComparisonCondition(type)); } }

Done this one.

I was just following the style in the existing implementations which are using ToArray in each function call... perhaps a boolean could have been passed there instead of passing entire array and checking if the type is 0th element in the array...

I think all this can be cleaned up (although this is just a template processing code), in the other issue that I have opened dotnet/coreclr#16745

WinCPP · 2018-03-05T16:51:48Z

src/mscorlib/shared/System/Numerics/Vector.cs

@@ -390,7 +390,7 @@ public unsafe Vector(T[] values, int index)
            }
            if (index < 0 || (values.Length - index) < Count)
            {
-                throw new IndexOutOfRangeException();
+                throw new IndexOutOfRangeException(SR.Format(SR.Arg_InsufficientNumberOfElements, Vector<T>.Count, nameof(values)));


Reintroduced this as per comment here from @4creators in one of the previous commits...

dotnet/corefx#24343

jkotas · 2018-03-05T21:07:33Z

src/mscorlib/System.Private.CoreLib.csproj

@@ -34,6 +34,7 @@
    <SignAssembly>true</SignAssembly>
    <DelaySign>true</DelaySign>
    <DefineConstants>$(DefineConstants);CORECLR;_USE_NLS_PLUS_TABLE;RESOURCE_SATELLITE_CONFIG;CODE_ANALYSIS_BASELINE</DefineConstants>
+    <DefineConstants Condition="'$(TargetGroup)'=='netcoreapp' OR '$(TargetGroup)'=='uap' OR '$(TargetGroup)'=='uapaot'">$(DefineConstants);netcoreapp</DefineConstants>


This can defined unconditionally here. The condition is not necessary.

dotnet/corefx#24343

eerhardt

LGTM. Thanks @WinCPP !

eerhardt · 2018-03-07T00:42:16Z

OSX is failing everywhere. Not blocking on it. Merging.

* CoreFX #24343 Vector using Span dotnet/corefx#24343 * CoreFX #24343 Vector using Span dotnet/corefx#24343 * CoreFX #24343 Vector using Span dotnet/corefx#24343 Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* CoreFX #24343 Vector using Span dotnet/corefx#24343 * CoreFX #24343 Vector using Span dotnet/corefx#24343 * CoreFX #24343 Vector using Span dotnet/corefx#24343 Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

ahsonkhan · 2018-03-08T21:27:34Z

src/mscorlib/System.Private.CoreLib.csproj

@@ -33,7 +33,7 @@
    <GenerateTargetFrameworkAttribute>false</GenerateTargetFrameworkAttribute>
    <SignAssembly>true</SignAssembly>
    <DelaySign>true</DelaySign>
-    <DefineConstants>$(DefineConstants);CORECLR;_USE_NLS_PLUS_TABLE;RESOURCE_SATELLITE_CONFIG;CODE_ANALYSIS_BASELINE</DefineConstants>
+    <DefineConstants>$(DefineConstants);CORECLR;_USE_NLS_PLUS_TABLE;RESOURCE_SATELLITE_CONFIG;CODE_ANALYSIS_BASELINE;netcoreapp</DefineConstants>


Apologies for the late review.
I don't think we should define this constant. It shouldn't be necessary.

cc @jkotas

See #16733 (comment)

Basically, this code is compiled here for netcoreap, and in CoreFX for netfx and netstandard. So just !netstandard isn't going to be enough.

Thanks for linking to the previous discussion. I had missed it. Can we instead define netcoreapp on the corefx side instead? Having an #if netcoreapp within coreclr seems weird.

IMO it only seems weird until you realize that this file is "shared" outside of coreclr.

The other way to make it work is to say:

#if !netfx and !netstandard

And then define netfx and netstandard in CoreFX.

But then it would seem just as weird that you'd have #if !netfx. This is coreclr - we shouldn't have any "netfx" code in it....

But then it would seem just as weird that you'd have #if !netfx. This is coreclr - we shouldn't have any "netfx" code in it....

Agreed. I retract my concern :)

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

src/mscorlib/shared/System/Numerics/Vector.cs

@@ -2,9 +2,13 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+#if netcoreapp


Why can't we use the existing netstandard constant here (for instance just like https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/MemoryExtensions.cs#L9).

We discussed that here: #16733 (comment)

ahsonkhan · 2018-03-08T21:28:10Z

src/mscorlib/shared/System/Numerics/Vector.cs

@@ -2,9 +2,13 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+#if netcoreapp
+using Internal.Runtime.CompilerServices;


nit: put this after all the System.* using directives

I thought we sort them alphabetically...

Yes, but for ones other than System (like internal), we group them separately (with system first). At least that is what I have seen (and followed) elsewhere.

So, something like:

using System.X; using System.Y; using System.Z; using Internal.A; using Internal.B; using Internal.C;

Between, this PR is already merged and closed. So perhaps we can do all this in the optimization issue along with other clean up?

Of course. That would be great.

ahsonkhan · 2018-03-08T21:34:53Z

src/mscorlib/shared/System/Numerics/Vector.cs

+
+#if netcoreapp
+        /// <summary>
+        /// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.


nit: use proper xml formatting (see
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Buffers/OwnedMemory.cs#L16 / https://github.com/dotnet/coreclr/blob/master/src/mscorlib/shared/System/Span.cs#L90).

The span must contain at least Vector<typeparamref name="T"/>.Count elements.

Yup. But I kept it same like other instances in the file for consistency. E.g. here i.e. below code,

/// Constructs a vector from the given array. The size of the given array must be at least Vector'T.Count. /// </summary> [Intrinsic] public unsafe Vector(T[] values) : this(values, 0) { }

Do we want to fix this as part of the optimization issue #16745?

This can be addressed separately and we can make the changes to the whole file at the same time. Good point about consistency with other places in the file.

ahsonkhan · 2018-03-08T21:37:56Z

src/mscorlib/shared/System/Numerics/Vector.cs

+        public Vector(Span<T> values)
+            : this()
+        {
+            if ((typeof(T) == typeof(Byte))


What about T == char?

Also, nit, can you use, byte/sbyte,short/long/etc, instead (to be consistent with the default formatting VS suggests)?

Vector doesn't support char because it isn't a number type.

Vector doesn't support char because it isn't a number type.

Ah I see. Tyvm.

The type names Byte etc. come from existing template generation code here. I did see VS suggesting me to use byte, etc. instead, but then I didn't purposely do that cleanup as it is not really a part of this PR.

That's fair. It doesn't follow our coding guidelines, but like you mentioned, this should be fixed separately. Thanks!

* CoreFX #24343 Vector using Span dotnet/corefx#24343 * CoreFX #24343 Vector using Span dotnet/corefx#24343 * CoreFX #24343 Vector using Span dotnet/corefx#24343 Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

WinCPP mentioned this pull request Mar 3, 2018

Issue #24343 Vector Ctor using Span dotnet/corefx#26499

Merged

jkotas reviewed Mar 3, 2018

View reviewed changes

ahsonkhan added the area-System.Numerics label Mar 4, 2018

ahsonkhan assigned WinCPP Mar 4, 2018

WinCPP force-pushed the fx_issue_24343 branch from 357e213 to 957113f Compare March 4, 2018 16:17

fiigii reviewed Mar 4, 2018

View reviewed changes

eerhardt reviewed Mar 5, 2018

View reviewed changes

WinCPP commented Mar 5, 2018

View reviewed changes

CoreFX dotnet#24343 Vector using Span

1912fe1

dotnet/corefx#24343

WinCPP force-pushed the fx_issue_24343 branch from c9cb332 to 1912fe1 Compare March 5, 2018 20:17

CoreFX dotnet#24343 Vector using Span

52e95ad

dotnet/corefx#24343

jkotas reviewed Mar 5, 2018

View reviewed changes

CoreFX dotnet#24343 Vector using Span

71d5215

dotnet/corefx#24343

eerhardt approved these changes Mar 5, 2018

View reviewed changes

eerhardt merged commit 7dc0649 into dotnet:master Mar 7, 2018

ahsonkhan reviewed Mar 8, 2018

View reviewed changes

WinCPP mentioned this pull request Jan 31, 2020

Optimize methods in System.Numerics.Vector<T> dotnet/runtime#9875

Open

CoreFX #24343 Vector Ctor using Span #16733

CoreFX #24343 Vector Ctor using Span #16733

Conversation

WinCPP commented Mar 3, 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

WinCPP Mar 5, 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 Mar 3, 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

jkotas commented Mar 3, 2018

WinCPP commented Mar 3, 2018

jkotas commented Mar 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Mar 5, 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

eerhardt left a comment

Choose a reason for hiding this comment

eerhardt commented Mar 7, 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

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

ahsonkhan 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

ahsonkhan 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 5, 2018 •

edited

Loading

jkotas Mar 3, 2018 •

edited

Loading

jkotas Mar 5, 2018 •

edited

Loading

ahsonkhan Mar 8, 2018 •

edited

Loading

ahsonkhan Mar 8, 2018 •

edited

Loading