Skip to content
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

Add SpanExtensions.AsVector #23688

Closed
KrzysztofCwalina opened this issue Sep 29, 2017 · 13 comments
Closed

Add SpanExtensions.AsVector #23688

KrzysztofCwalina opened this issue Sep 29, 2017 · 13 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@KrzysztofCwalina
Copy link
Member

Final/Approved Proposal

https://github.com/dotnet/corefx/issues/24343#issuecomment-349418624

namespace System.Numerics
{
    public struct Vector<T>
    {
        // Existing API
        public Vector(T[] values);
        // Proposed API
        public Vector(Span<T> values);
    }
}

Original Proposal

It's quite common to want to convert a Span to Vector. It's possible to open code such conversion, but the code is a bit tricky and people do it commonly enough that we shoudl just add a helper to SpanExtensions

public static Vector<T> AsVector<T>(this Span<T> slice) {
    if (slice.Length < Vector<T>.Count) throw new ArgumentOutOfRangeException();
    return Unsafe.ReadUnaligned<Vector<T>>(ref Unsafe.As<T, byte>(ref slice.DangerousGetPinnableReference()));
}

cc: @jkotas, @mellinoe, @ahsonkhan, @benaadams, @stephentoub

@mellinoe
Copy link
Contributor

Is converting your Span<T> into a Span<Vector<T>> a viable / safe option? That would let you use the span normally, through the indexer.

@benaadams
Copy link
Member

Don't need the ReadUnaligned?

public static Vector<T> AsVector<T>(this Span<T> slice) where T : struct
{
    if (slice.Length < Vector<T>.Count) throw new ArgumentOutOfRangeException();
    return Unsafe.As<T, Vector<T>>(ref slice.DangerousGetPinnableReference());
}

What would the usage be? Something like:

while (span.Length >= Vector<byte>.Count)
{
    var vec = span.AsVector();
    // stuff with vec
    span = span.Slice(0, Vector<byte>.Count);
}

Is converting your Span into a Span<Vector> a viable / safe option? That would let you use the span normally, through the indexer.

Something like?

var vectorSpan = span.NonPortableCast<byte, Vector<byte>>();
for(var i = 0; i < vectorSpan.Length; i++)
{
    var vec = vectorSpan[i];
    // stuff with vec
}

@mellinoe
Copy link
Contributor

Something like?

Right, and then you could just read and write directly to the indexer.

vectorSpan[i] ^= mask;
vectorSpan[i] += vectorSpan[i + 1];
// Etc.

@benaadams
Copy link
Member

benaadams commented Sep 29, 2017

Merging the two... What about AsVectorSpan so you don't need to invoke NonPortableCast (as a user)?

public static Span<Vector<T>> AsVectorSpan<T>(this Span<T> slice) where T : struct
{
    return slice.NonPortableCast<T, Vector<T>>();
}

@stephentoub
Copy link
Member

stephentoub commented Sep 29, 2017

With the earlier example:

while (span.Length >= Vector<byte>.Count)
{
    Vector<byte> vec = span.AsVector();
    // stuff with vec
    span = span.Slice(0, Vector<byte>.Count);
}

presumably it's followed by a loop on the remaining bytes:

while (span.Length >= Vector<byte>.Count)
{
    Vector<byte> vec = span.AsVector();
    // stuff with vec
    span = span.Slice(0, Vector<byte>.Count);
}

for (int i = 0; i < span.Length; i++)
{
    // stuff with span[i]
}

What does that look like with this Span<Vector<T>> version?

@mellinoe
Copy link
Contributor

I think it would be something like this:

Span<T> span;
Span<Vector<T>> vSpan = Convert(span);
for (int i = 0; i < vSpan.Length; i++)
{
    // Do stuff with vSpan[i]
}

span = span.Slice(Vector<T>.Count * vSpan.Length);
for (int i = 0; i < span.Length; i++)
{
    // stuff with span[i]
}

@benaadams
Copy link
Member

What does that look like with this Span<Vector> version?

Depends if slice first or hard fail on conversion

var vSpan = span.Slice(0, span.Length % Vector<byte>.Count).AsVectorSpan();
for(var i = 0; i < vSpan.Length; i++)
{
    var vec = vSpan[i];
    // stuff with vec
}

span = span.Slice(Vector<byte>.Count * vSpan.Length);
for (int i = 0; i < span.Length; i++)
{
    // stuff with span[i]
}

or as part of a Try call that does a partial amount

if (span.TryAsVectorSpan(out var vSpan))
{
    for (var i = 0; i < vSpan.Length; i++)
    {
        var vec = vSpan[i];
        // stuff with vec
    }

    span = span.Slice(Vector<byte>.Count * vSpan.Length);
}

for (int i = 0; i < span.Length; i++)
{
    // stuff with span[i]
}

With a TryAsVectorSpan

public static bool TryAsVectorSpan<T>(this Span<T> slice, out Span<Vector<T>> vectorSpan) where T : struct
{
    var count = slice.Length % Vector<byte>.Count;
    if (count > 0)
    {
        vectorSpan = slice.Slice(0, count).NonPortableCast<T, Vector<T>>();
        return true;
    }

    vectorSpan = default;
    return false;
}

May also have predetermined fixed sized Vector processing that has no extra elements

public static Span<Vector2> AsVector2Span(this Span<byte> slice) 
{
    return slice.NonPortableCast<byte, Vector2>();
}
public static Span<Vector3> AsVector3Span(this Span<byte> slice) 
{
    return slice.NonPortableCast<byte, Vector3>();
}
public static Span<Vector4> AsVector4Span(this Span<byte> slice) 
{
    return slice.NonPortableCast<byte, Vector4>();
}

Probably the extensions should live in namespace System.Numerics?

@benaadams
Copy link
Member

.AsVector forces it to local var; which has the advantage of more likely dealing with vector registers; however would require the (Vector size) bounds check to be hoisted across slicing which may be more tricky? Also updating the Vector in the span would be harder for writes.

.AsVectorSpan the bounds check (at Vector size) is manually hoisted via the api; and write backs are easier; so if you wanted to apply a in-place masking on a stream you could do

for (var i = 0; i < vSpan.Length; i++)
{
   var vec = vSpan[i];
   vec ^= mask;
   vSpan[i] = vec;
}

Though it may lead to more direct operations on the Vector elements, so shunting from memory and registers more?

@ahsonkhan
Copy link
Member

@KrzysztofCwalina, is the proposal ready for review?

@terrajobst
Copy link
Member

terrajobst commented Dec 5, 2017

Video

We should align it with the existing APIs, so I'd rather make this a ctor on Vector<T> itself:

namespace System.Numerics
{
    public struct Vector<T>
    {
        // Existing API
        public Vector(T[] values);
        // Proposed API
        public Vector(Span<T> values);
    }
}

@karelz
Copy link
Member

karelz commented Dec 5, 2017

FYI: The API review discussion was recorded - see https://youtu.be/BI3iXFT8H7E?t=5251 (11 min duration)

@karelz
Copy link
Member

karelz commented Dec 5, 2017

Top post updated with approved API shape above https://github.com/dotnet/corefx/issues/24343#issuecomment-349418624.

@WinCPP
Copy link
Contributor

WinCPP commented Jan 21, 2018

To make sure I understand the ask and, the conclusion in the recording... this is a constructor that "copies" the contents of the span into the vector; no re-interpretation of span's is expected here...


EDIT: I had code ready, based on the understanding, I have raised the PR. Kindly review and let me know.

WinCPP referenced this issue in WinCPP/coreclr Mar 1, 2018
WinCPP referenced this issue in WinCPP/coreclr Mar 3, 2018
WinCPP referenced this issue in WinCPP/coreclr Mar 4, 2018
WinCPP referenced this issue in WinCPP/coreclr Mar 5, 2018
WinCPP referenced this issue in WinCPP/coreclr Mar 5, 2018
WinCPP referenced this issue in WinCPP/coreclr Mar 5, 2018
eerhardt referenced this issue in dotnet/coreclr Mar 7, 2018
* CoreFX #24343 Vector using Span

dotnet/corefx#24343

* CoreFX #24343 Vector using Span

dotnet/corefx#24343

* CoreFX #24343 Vector using Span

dotnet/corefx#24343
dotnet-bot referenced this issue in dotnet/corert Mar 7, 2018
* 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>
jkotas referenced this issue in dotnet/corert Mar 7, 2018
* 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>
dotnet-bot referenced this issue in dotnet/corefx Mar 8, 2018
* 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>
stephentoub referenced this issue in dotnet/corefx Mar 8, 2018
* 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>
jkotas referenced this issue in jkotas/corefx Mar 10, 2018
kbaladurin referenced this issue in kbaladurin/corert Mar 15, 2018
* 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>
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

9 participants