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 all 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>
2 changes: 1 addition & 1 deletion src/mscorlib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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

cc @jkotas

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

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()
{
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