Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

CoreFX #24343 Vector Ctor using Span #16733

Merged
merged 3 commits into from
Mar 7, 2018
Merged

CoreFX #24343 Vector Ctor using Span #16733

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1912fe1
CoreFX #24343 Vector using Span
WinCPP Mar 1, 2018
52e95ad
CoreFX #24343 Vector using Span
WinCPP Mar 1, 2018
71d5215
CoreFX #24343 Vector using Span
WinCPP Mar 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/mscorlib/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3715,4 +3715,7 @@
<data name="Argument_OverlapAlignmentMismatch" xml:space="preserve">
<value>Overlapping spans have mismatching alignment.</value>
</data>
</root>
<data name="Arg_InsufficientNumberOfElements" xml:space="preserve">
<value>At least {0} element(s) are expected in the parameter "{1}".</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/mscorlib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

<!-- We don't use any of MSBuild's resolution logic for resolving the framework, so just set these two properties to any folder that exists to skip
the GenerateReferenceAssemblyPaths task (not target) and to prevent it from outputting a warning (MSB3644). -->
<_TargetFrameworkDirectories>$(MSBuildThisFileDirectory)/Documentation</_TargetFrameworkDirectories>
Expand Down
28 changes: 27 additions & 1 deletion src/mscorlib/shared/System/Numerics/GenerationConfig.ttinclude
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
<#@ import namespace="System.Linq" #>
<#@ import namespace="System.Collections.Generic" #>
<#@ import namespace="System.Text" #>
<#+
/* This file includes static data used as compilation configuration for the rest of the code generation.
It is shared here to ensure that all generated code compiles with the same constants and configurations. */
Expand Down Expand Up @@ -144,4 +145,29 @@
string keyword = (type == allTypes.ToArray()[0]) ? "if" : "else if";
return string.Format("{0} (typeof(T) == typeof({1}))", keyword, type.Name);
}
#>

public string MakeTypeComparisonCondition(Type type)
{
return string.Format("(typeof(T) == typeof({0}))", type.Name);
}

public string GenerateIfConditionAllTypes(IEnumerable<Type> allTypes)
{
StringBuilder sbuilder = new StringBuilder();
bool firstTime = true;
foreach (var type in allTypes)
{
if (firstTime)
{
sbuilder.Append("if (").Append(MakeTypeComparisonCondition(type));
firstTime = false;
}
else
{
sbuilder.AppendLine().Append(" || ").Append(MakeTypeComparisonCondition(type));
}
}
sbuilder.Append(")");
return sbuilder.ToString();
}
#>
37 changes: 36 additions & 1 deletion src/mscorlib/shared/System/Numerics/Vector.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed that here: #16733 (comment)

using Internal.Runtime.CompilerServices;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@WinCPP WinCPP Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we sort them alphabetically...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course. That would be great.

#endif
using System.Globalization;
using System.Numerics.Hashing;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;

namespace System.Numerics
Expand Down Expand Up @@ -386,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)));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if (Vector.IsHardwareAccelerated)
Expand Down Expand Up @@ -763,6 +767,37 @@ private Vector(ref Register existingRegister)
{
this.register = existingRegister;
}

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

@ahsonkhan ahsonkhan Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// </summary>
public Vector(Span<T> values)
: this()
{
Copy link
Member

@jkotas jkotas Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

if ((typeof(T) == typeof(Byte))
Copy link
Member

@ahsonkhan ahsonkhan Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about T == char?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ah I see. Tyvm.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

|| (typeof(T) == typeof(SByte))
|| (typeof(T) == typeof(UInt16))
|| (typeof(T) == typeof(Int16))
|| (typeof(T) == typeof(UInt32))
|| (typeof(T) == typeof(Int32))
|| (typeof(T) == typeof(UInt64))
|| (typeof(T) == typeof(Int64))
|| (typeof(T) == typeof(Single))
|| (typeof(T) == typeof(Double)))
{
if (values.Length < Count)
{
throw new IndexOutOfRangeException(SR.Format(SR.Arg_InsufficientNumberOfElements, Vector<T>.Count, nameof(values)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

}
this = Unsafe.ReadUnaligned<Vector<T>>(ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(values)));
}
else
{
throw new NotSupportedException(SR.Arg_TypeNotSupported);
}
}
#endif
#endregion Constructors

#region Public Instance Methods
Expand Down
28 changes: 27 additions & 1 deletion src/mscorlib/shared/System/Numerics/Vector.tt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,13 @@
<#@ import namespace="System.Runtime.InteropServices" #>
<#@ include file="GenerationConfig.ttinclude" #><# GenerateCopyrightHeader(); #>

#if netcoreapp
using Internal.Runtime.CompilerServices;
#endif
using System.Globalization;
using System.Numerics.Hashing;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;

namespace System.Numerics
Expand Down Expand Up @@ -198,7 +202,7 @@ namespace System.Numerics
}
if (index < 0 || (values.Length - index) < Count)
{
throw new IndexOutOfRangeException();
throw new IndexOutOfRangeException(SR.Format(SR.Arg_InsufficientNumberOfElements, Vector<T>.Count, nameof(values)));
}

if (Vector.IsHardwareAccelerated)
Expand Down Expand Up @@ -283,6 +287,28 @@ namespace System.Numerics
{
this.register = existingRegister;
}

#if netcoreapp
/// <summary>
/// Constructs a vector from the given span. The span must contain at least Vector'T.Count elements.
/// </summary>
public Vector(Span<T> values)
: this()
{
<#=GenerateIfConditionAllTypes(supportedTypes)#>
{
if (values.Length < Count)
{
throw new IndexOutOfRangeException(SR.Format(SR.Arg_InsufficientNumberOfElements, Vector<T>.Count, nameof(values)));
}
this = Unsafe.ReadUnaligned<Vector<T>>(ref Unsafe.As<T, byte>(ref MemoryMarshal.GetReference(values)));
}
else
{
throw new NotSupportedException(SR.Arg_TypeNotSupported);
}
}
#endif
#endregion Constructors

#region Public Instance Methods
Expand Down