Rewrite of System.Numerics.Tensors to allow for more code sharing, various correctness fixes, and stability improvements

[tannergooding]

No description provided.

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[tarekgh]

                dstSpan[i] = valuesSpan[index++];

should we check the valueSpan length before indexing and throw proper exception?

---

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:663 in eb42165. [](commit_id = eb42165, deletion_comment = False)

[michaelgsharp]

                if (tensors[0].Rank != tensors[i].Rank)

nit: would make sense to store tensors[0].Rank in a local variable and use it? or you think jit optimize that anyway?

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:217 in eb42165. [](commit_id = eb42165, deletion_comment = False)

No idea if the jit will optimize it or not, but just for clarity sake its probably better to store it locally.

[michaelgsharp]

                    if (j != dimension)

Why we excluding only the dimension check here? should actually the for loop go up to Min(tensors[0].Rank, dimension)? I may be missing something :-)

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:221 in eb42165. [](commit_id = eb42165, deletion_comment = False)

For Concatenate, all dimensions must be exactly the same except for the dimension we are concatenating on. So we skip it in that loop since we don't care if its equal.

[michaelgsharp]

            T[] array = enumerable.ToArray();

nit: move this line outside the pinned check? similar comment for other Create method.

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:344 in eb42165. [](commit_id = eb42165, deletion_comment = False)

Yeah, here it can be moved out.

[tarekgh]

                    ThrowHelper.ThrowArgument_PermuteAxisOrder();

maybe moving this check in the beginning of the else block would be reasonable? just to avoid the creating the spans? of course you can add to the check !dimensions.IsEmpty

---

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:1430 in eb42165. [](commit_id = eb42165, deletion_comment = False)

[tarekgh]

                    newLengths[i] = tensor.Lengths[dimensions[i]];

would the values in the dimensions be trusted or it is ok to get out of range when indexing here?

---

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:1434 in eb42165. [](commit_id = eb42165, deletion_comment = False)

[michaelgsharp]

                    ThrowHelper.ThrowArgument_PermuteAxisOrder();

maybe moving this check in the beginning of the else block would be reasonable? just to avoid the creating the spans? of course you can add to the check !dimensions.IsEmpty

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:1430 in eb42165. [](commit_id = eb42165, deletion_comment = False)

Good call. Fixed.

[michaelgsharp]

                    newLengths[i] = tensor.Lengths[dimensions[i]];

would the values in the dimensions be trusted or it is ok to get out of range when indexing here?

Refers to: src/libraries/System.Numerics.Tensors/src/System/Numerics/Tensors/netcore/Tensor.cs:1434 in eb42165. [](commit_id = eb42165, deletion_comment = False)

If they provide invalid dimensions its ok for them to get an error. It would probably be more clear though if we explicitly checked and then threw a more helpful error. I'll create an issue/todo for that.

[tannergooding]

This is confusing :-)

I expect TensorOperation.GreaterThanAny return the actual value we need to return and not to negate the result. > Otherwise, the naming is not correct.

This is the naming convention that already shipped for these APIs on Vector64/128/256/512 and TensorPrimitives, we've had it for several years. We have *All, *Any, and *None APIs that answer the question "do all, any, or none of the elements meet the * condition". It is similar in nature to enumable.All() and enumerable.Any() from LINQ.

Thus, the bool is the value we need to return and is what TensorOperation.GreaterThanAny is computing. The internal only Invoke wrapper is what exits on the first false condition and is a general purpose helper to avoid code duplication and reduce overall complexity/bug risk. The comment just above and in the also internal only operation helper then explains that and why it has to be negated.

Anything else requires exposing another Invoke method with specialized exit conditions, which adds additional risk, complexity, and duplicates quite a bit of code we want to minimize.

[tannergooding]

-- As a side note, the way the review comments are being made does not attach them to the code like a normal review comment. This makes it very difficult to actually reply, to find out what was actually being commented on, and to know when something has or hasn't been resolved.

[tannergooding]

I assume if any exception thrown before disposing will cause the buffer to be garbage collected. is that right? I mean we don't need to have try-finally here to ensure disposing.

It's fairly typical for us to not call ArrayPool<T>.Shared.Return if an exception is thrown, this is done all throughout the BCL.

Returning a rented array to the pool is not required and is simply best practice, but the overhead of the exception, risk for use after free bugs, and other considerations means we typically only return on the success path.

[tarekgh]

As a side note, the way the review comments are being made does not attach them to the code like a normal review comment. This makes it very difficult to actually reply, to find out what was actually being commented on, and to know when something has or hasn't been resolved.

Sorry about that, I am using codeflow and was assuming putting the comments attached to the code. It is weird as the first comments were correctly attached. I'll try to ensure comments attached to the code.

[tarekgh]

Will these tests get done before merging?

[tannergooding]

No, they would be for additional APIs that need to be exposed after the Build preview

[tarekgh]

Added minor comments, LGTM!

[tannergooding]

/backport to release/10.0-preview4

[github-actions]

Started backporting to release/10.0-preview4: https://github.com/dotnet/runtime/actions/runs/14629464290