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

Updating Vector*.IsHardwareAccelerated to be recursive #69578

Merged

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented May 19, 2022

This resolves #38162

This updates the various IsHardwareAccelerated APIs to be recursive, just as they are for the IsSupported APIs. This ensures that they return the same result whether invoked directly or indirectly (such as via reflection). This ensures that the debugger and other scenarios report a consistent worldview.

@ghost
Copy link

ghost commented May 19, 2022

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #69576

This updates the various IsHardwareAccelerated APIs to be recursive, just as they are for the IsSupported APIs. This ensures that they return the same result whether invoked directly or indirectly (such as via reflection). This ensures that the debugger and other scenarios report a consistent worldview.

Author: tannergooding
Assignees: -
Labels:

area-System.Numerics

Milestone: -

@tannergooding
Copy link
Member Author

tannergooding commented May 19, 2022

@davidwrighton, looks like ReadyToRun for Vector256.IsHardwareAccelerated is returning false for the indirect invocation (meaning its baked into corelib as false).

Is that expected? We don't appear to do the same for Vector<T> or Avx/Avx2.

That is, I see the following where the first column is a direct call (e.g. Vector.IsHardwareAccelerated) and the second column is an indirect call (e.g. typeof(Vector).GetMethod("get_IsHardwareAccelerated").Invoke(null, null))

Vector:    True;  True
Vector64:  False; False
Vector128: True;  True
Vector256: True;  False
SSE:       True;  True
AVX:       True;  True
AVX2:      True;  True

@tannergooding
Copy link
Member Author

CC. @dotnet/crossgen-contrib for help with the unexpected failure above.

@davidwrighton
Copy link
Member

How odd. Well, I'll look into this. Will take a bit for me to clear my schedule.

@jeffhandley
Copy link
Member

@davidwrighton Do you think there's something lurking in these unexpected failures that we need to make sure to address in 7.0?

@danmoseley
Copy link
Member

I think I reported this originally - we're trying to make writing vectorized code more accessible in .NET 7, and this is likely to confuse initially as it did me, so it would be nice to get it in .NET 7.

@stephentoub
Copy link
Member

@davidwrighton, are you still able to look at this?

@danmoseley
Copy link
Member

@davidwrighton ?

@davidwrighton
Copy link
Member

Sorry, this fell off my radar. I'm looking at it today.

…nsics in CoreLib

- The functions which detect if an intrinsic is actually available are allowed to have behavior which differs based on which intrinsics are available at runtime
@tannergooding
Copy link
Member Author

@fanyang-mono, could you assist with the changes needed for Mono here?

I went and looked at intrinsics.c and simd-intrinsics.c, but it looks like the get_IsHardwareAccelerated methods already have handling and similar handling to get_IsSupported, so its not clear what's missing and causing it to fail on the recursion rather than always creating a constant true/false node

@fanyang-mono
Copy link
Member

@tannergooding I will see if I have time to get to it today or tmr. Otherwise, I will forward this to another person on mono team to handle. Because I will be on vacation starting from this Friday for two weeks.

@tannergooding
Copy link
Member Author

Thanks much!

@danmoseley
Copy link
Member

@fanyang-mono did you manage to identify someone who could help with the mono side?

@fanyang-mono
Copy link
Member

@vargaz Could you help out with this while I am on vacation?

@tannergooding
Copy link
Member Author

ping @fanyang-mono, @vargaz on this.

I imagine its too late for .NET 7 at this point, but it would still be nice to get fixed

@danmoseley
Copy link
Member

@tannergooding is the CoreCLR change mergeable on its own?

@danmoseley
Copy link
Member

Also, I wouldn't rule out .NET 7 if the risk is low, as it's a goal of the release to make it easy to use vectorization in your own code. This trips you up right away.

@tannergooding
Copy link
Member Author

@tannergooding is the CoreCLR change mergeable on its own?

Technically, yes. But there's no real point, IMO, if we aren't going to ship Corelib with the support since users will never trigger the code.

Also, I wouldn't rule out .NET 7 if the risk is low, as it's a goal of the release to make it easy to use vectorization in your own code. This trips you up right away.

I'm not sure I'd say this is a right away scenario. Calling this via reflection or indirectly such as via a delegate is expected to be an extremely rare/niche scenario and not the normal usage pattern.

I expect the debugger case is more likely, but we really need the Mono support as well.

@danmoseley
Copy link
Member

Ah. Agreed, I think the debugger case is the only one that is significant.

@danmoseley
Copy link
Member

cc @SamMonoRT since it's potentially 7.0

@vargaz
Copy link
Contributor

vargaz commented Sep 14, 2022

This will fix the mono interpreter failures:
vargaz@8be6b8d
Were there any other failures ?

@danmoseley
Copy link
Member

relevant failures eg

Assert failure(PID 7492 [0x00001d44], Thread: 5828 [0x16c4]): Assertion failed '!"Unhandled must expand intrinsic, throwing PlatformNotSupportedException"' in 'System.Runtime.Intrinsics.Vector256:get_IsHardwareAccelerated():bool' during 'Importation' (IL size 6; hash 0x068ee1df; FullOpts)

File: D:\a\_work\1\s\src\coreclr\jit\importer.cpp Line: 4938
Image: C:\h\w\A5E108DA\p\corerun.exe

and mono failures eg JIT.HardwareIntrinsics.X86.Bmi1

@tannergooding
Copy link
Member Author

tannergooding commented Sep 14, 2022

Its actually not clear to me why CI is asserting. It doesn't assert locally at all (and shouldn't since the only recursive API is get_IsHardwareAccelerated and that's handled).

However, I've added an extra safety path to see if that resolves the issue.

@tannergooding
Copy link
Member Author

Actually, realized the potential issue right as I typed that up. The path that could convert IsHardwareAccelerated to true was outside the FEATURE_HW_INTRINSICS path when it should've been "in" with the fallback handler always returning false.

@tannergooding
Copy link
Member Author

Were there any other failures ?

@vargaz, looks like still some Mono Linux failures where its unexpectedly getting System.PlatformNotSupportedException.


There is also a failure for RyuJIT x64/x86 Checked no_tiered_compilation. It's saying:

Assert failure(PID 7696 [0x00001e10], Thread: 5688 [0x1638]): Assertion failed '!"Unhandled must expand intrinsic, throwing PlatformNotSupportedException"' in 'System.Runtime.Intrinsics.Vector128:get_IsHardwareAccelerated():bool' during 'Importation' (IL size 6; hash 0x330ddaf5; FullOpts)

File: D:\a\_work\1\s\src\coreclr\jit\importer.cpp Line: 4938

@davidwrighton, this appears to be because these tests disable SSE2 and so the intrinsic returned is NI_IsSupported_Dynamic. I don't see any handling for this in the JIT, so what's supposed to happen for it in this recursive case?

@vargaz
Copy link
Contributor

vargaz commented Sep 15, 2022

The mono failures are also present in HEAD, so they are unrelated:
https://github.com/dotnet/runtime/runs/8369467985

@tannergooding
Copy link
Member Author

Failures are #75753 and #75606

@tannergooding tannergooding merged commit b40c3a3 into dotnet:main Sep 17, 2022
@tannergooding
Copy link
Member Author

@danmoseley, are you still wanting this backported to .NET 7?

For Vector64/128/256<T> this is "new API functionality". For Vector<T> this has been the behavior since it was released in 2014.

There have been only a couple user reports on this, so I'm not sure if it meets the bar or not.

@danmoseley
Copy link
Member

Yeah now we branched, I don't think it does. Thanks for driving it though.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 18, 2022
@tannergooding tannergooding deleted the ishardwareaccelerated-reflection branch June 14, 2023 15:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector.IsHardwareAccelerated result differs when called directly and called via reflection
7 participants