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

Vector.IsHardwareAccelerated result differs when called directly and called via reflection #38162

Closed
AndyAyersMS opened this issue Jun 19, 2020 · 8 comments · Fixed by #69578
Closed
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Jun 19, 2020

using System;
using System.Reflection;
using System.Numerics;

class X
{
    public static int Main()
    {
        bool hw1 = Vector.IsHardwareAccelerated;

        Assembly spc = typeof(object).Assembly;
        Type vecType = spc.GetType("System.Numerics.Vector");
        MethodInfo method = vecType.GetMethod("get_IsHardwareAccelerated");
        bool hw2 = (bool) method.Invoke(null, null);

        Console.WriteLine($"Direct call to Vector.IsHardwareAccelerated returns {hw1}");
        Console.WriteLine($"Reflection call to Vector.IsHardwareAccelerated returns {hw2}");

        return (hw1 == hw2 ? 100 : -1);
    }
}

results in

Direct call to Vector.IsHardwareAccelerated returns True
Reflection call to Vector.IsHardwareAccelerated returns False

Seems like we're not recognizing this as an intrinsic when it is jitted on its own.

category:cq
theme:vector-codegen
skill-level:intermediate
cost:medium

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels Jun 19, 2020
@AndyAyersMS AndyAyersMS added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Reflection labels Jun 19, 2020
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib @tannergooding

@AndyAyersMS
Copy link
Member Author

Saw this when looking at PMI results, where we force this method to be jitted.

@tannergooding
Copy link
Member

CC. @CarolEidt

This has always been like this as the methods aren't recursive (and we didn't actually add the mustExpand recursion support until HWIntrinsics in 3.0).

It shouldn't be particularly hard to fix, however.

@BruceForstall BruceForstall added this to the 5.0.0 milestone Jun 19, 2020
@BruceForstall BruceForstall removed the untriaged New issue has not been triaged by the area owner label Jun 19, 2020
@tannergooding tannergooding self-assigned this Jun 19, 2020
@tannergooding
Copy link
Member

I have the fix for this done locally however since IsHardwareAccelerated lives in Vector (rather than Vector<T>), it is included in R2R.

I though this just needed to be fTreatAsRegularMethodCall = TRUE in zapinfo, but then it isn't recognized as intrinsic at runtime and so it fails if R2R is enabled (the call ends up being recursive and not recognized as an intrinsic).

@tannergooding tannergooding modified the milestones: 5.0.0, Future Aug 10, 2020
@tannergooding
Copy link
Member

Moving this to future for the time being as the existing behavior goes back to .NET Framework and so I don't think it is particularly critical that we get it fixed for .NET 5.

I think I would need input from @davidwrighton or @jkotas as to what is missing for R2R to start working correctly with IsHardwareAccelerated being recursive.
As best as I can tell, there isn't any difference between it and the IsSupported checks in terms of attribution or handling, but I'm likely missing a check somewhere.

@tannergooding
Copy link
Member

The changes I had prepped are here, for reference: tannergooding@b6b555f

@jkotas
Copy link
Member

jkotas commented Aug 10, 2020

One problem that I can see in your delta is that the mustExpand expansion will kick in only for FEATURE_HW_INTRINSICS and so it will not work on ARM. I think it needs to be unconditional outside FEATURE_HW_INTRINSICS ifdef like the NI_IsSupported_True / NI_IsSupported_False for regular hardware intrinsics.

Also, you may need a matching change in Mono.

The zapper side of your delta looks fine.

@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@BruceForstall BruceForstall modified the milestones: Future, 6.0.0 Nov 7, 2020
@BruceForstall BruceForstall removed the JitUntriaged CLR JIT issues needing additional triage label Nov 7, 2020
@JulieLeeMSFT JulieLeeMSFT modified the milestones: 6.0.0, 7.0.0 Aug 9, 2021
echesakov added a commit to echesakov/runtime that referenced this issue Oct 4, 2021
@echesakov echesakov linked a pull request Oct 5, 2021 that will close this issue
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 4, 2022
@tannergooding tannergooding modified the milestones: 7.0.0, 8.0.0 Jun 16, 2022
@tannergooding
Copy link
Member

Moving to 8.0.0. This is in PR but not high priority and is dependent on someone from @dotnet/crossgen-contrib following up on the potential issue with the ISA flags

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
7 participants