Vector.As<TFrom, TTo>

[huoyaoyuan]

Closes #508

Not sure how to test it, because all the AsVectorXXX are not tested.

Questions:
Can we tread Unsafe.As to be reliably optimized by JIT and reduce the requirement for [Intrinsic]?
What's the simplest way to see the JIT and inlining decision on methods in CoreLib? jitdump or disasmo?

[dotnet-issue-labeler]

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, to 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]

Tagging subscribers to this area: @tannergooding, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Not sure how to test it, because all the AsVectorXXX are not tested.

Questions:
Can we tread Unsafe.As to be reliably optimized by JIT and reduce the requirement for [Intrinsic]?
What's the simplest way to see the JIT and inlining decision on methods in CoreLib? jitdump or disasmo?

Author:	huoyaoyuan
Assignees:	-
Labels:	area-System.Numerics, new-api-needs-documentation
Milestone:	-

[gfoidl]

JIT and inlining decision on methods in CoreLib? jitdump

IMHO yes, with COMPlus_JitDump set to the methodname -- you'll get a lot of (valuable) info, among them the JIT's reasoning for inlining (or not).

[huoyaoyuan]

I've tried JITDump but I remember I didn't get result for generic methods. (Or just generic for reference types)

[SingleAccretion]

Can we tread Unsafe.As to be reliably optimized by JIT and reduce the requirement for [Intrinsic]?

No, unfortunately not. Issues tend to crop up when you have things have their address taken in one way or another. This method will have to either be intrinsified (probably better in terms of Jit throughput and potentially CQ too) or use the existing AsXXX methods and a bunch of typeof(T) checks (no need to modify the native side).

What's the simplest way to see the JIT and inlining decision on methods in CoreLib? jitdump or disasmo?

For such simple methods that will be inlined unless you're out of budget, I'd use Disasmo (optionally with the dump option set). I'd write two methods that use Vector<T> and Ass somewhat heavily and compare the output between the existing methods and the generic one. The compilation problem should be solvable by decorating the method with aggressive inlining and then calling it from a non-generic wrapper.

[tannergooding]

I don' think this assert is necessary. If the size, in bytes, of Vector<T> vs Vector<U> varies then we have much bigger problems in the JIT 😄

[tannergooding]

Could you update the top post with a comment similar to: "This resolves #508"?

Not sure how to test it, because all the AsVectorXXX are not tested.

I'd recommend just adding a few tests that validate converting T to U results in a bitwise equivalent value. You can always use CopyTo to get the raw bytes and then do a SequenceEqual comparison to validate correctness.
At a minimum, this should likely cover float <-> int as well as cases like int <-> ulong.
Likewise, a few test should be added validating that conversion to and from types like bool throws.

[tannergooding]

I think it was missed in API review, so I'll see if I can bring it up quickly today, but I think this should be an extension method like it is for Vector128<T> and Vector256<T>

[tannergooding]

Having this be an extension method was approved in API review.

[tannergooding]

Changes look good to me. It would just be good to get some basic tests covering a successful case and an error with an invalid type for TFrom and TTo

[tannergooding]

This still needs a test for the positive case, it should be mergeable after that is added.

[huoyaoyuan]

Does it need to evaluate the content of vector? Or just 0 is fine?

[tannergooding]

Does it need to evaluate the content of vector? Or just 0 is fine?

Ideally it would test it with an actual value to assert there aren't any bugs or accidental conversions (now or in the future) and that it is actually a bit-cast.
The test can always be something simple like:

var x = new Vector<int>(new int[] { 1, 2, 3, 4, 5, 6, 7, 8 });
var y = x.As<int, float>();

var xBytes = new byte[Vector<T>.Count * 4];
x.CopyTo(xBytes);

var yBytes = new byte[Vector<T>.Count * 4];
y.CopyTo(yBytes);

AssertEqual(xBytes, yBytes);

And repeat for a few cases. The ones that are likely most interesting to cover are same size to same size (e.g. float<->int and int<->uint) and same size to different size (e.g. double<->int and long<->int).

[tannergooding]

@echesakovMSFT, could you give the HWIntrinsic changes a quick secondary review?

[echesakov]

LGTM

[tannergooding]

@huoyaoyuan, I'm investigating the alpine failure.

I'm also going to quickly close and reopen the PR so the CI jobs pick up latest master and therefore the System.Linq.Expressions test fix

[tannergooding]

The issue is that this is actually under a #if defined(TARGET_XARCH) already (defined on L385) and so this is never hit on arm64.

The simplest fix is to remove the #if defined(TARGET_XARCH) here and then copy the following logic into the #elif defined(TARGET_ARM64) on L446

    switch (intrinsic)
    {
        case NI_VectorT128_As:
        {
            unsigned  retSimdSize;
            var_types retBaseType = getBaseTypeAndSizeOfSIMDType(sig->retTypeSigClass, &retSimdSize);

            if (!varTypeIsArithmetic(retBaseType) || (retSimdSize == 0))
            {
                // We get here if the return type is an unsupported type
                return nullptr;
            }
            break;
        }

        default:
        {
            // Most intrinsics have some path that works even if only AdvSimd is available
            break;
        }
    }

[huoyaoyuan]

Got it. I'll look at the structure and find the opportunity to share code.

[huoyaoyuan]

I reordered the ifdef block into two. One for general instruction set availability, one for special logic for each instruction. Is this sure?

[tannergooding]

It looks correct. I think this will be good to merge if CI passes

[tannergooding]

Thanks for the PR @huoyaoyuan!