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

JIT: Added SVE GetFfr, SetFfr, LoadVectorFirstFaulting, GatherVectorFirstFaulting #105595

Merged
merged 88 commits into from
Aug 2, 2024

Conversation

TIHan
Copy link
Member

@TIHan TIHan commented Jul 27, 2024

Contributes to #99957

Same as #104502 but with a separate impl of BoundedMemory specifically used for SVE first faulting APIs.

TIHan and others added 30 commits July 2, 2024 12:54
…ly without the FirstFaulting test. Added SveFfrTest template.
somewhat workable

code cleanup

Remove FFR

Add all the GetFfr*

wip

Work with MskCns() model

Use physReg approach

Remove commented prototypes

working

Remove bunch of unnecessary code

Remove SpecialImport from GetFFR/SetFFR/LoadFirstFaulting

some more code cleanup

some fixup
private const string SystemNative = "libSystem.Native";

// NOTE: Shim returns null pointer on failure, not non-null MAP_FAILED sentinel.
[DllImport(SystemNative, EntryPoint = "SystemNative_MMap", SetLastError = true)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you should be able to access Interop.Libraries.SystemNative here for the lib name constant

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe just consume Interop.MMap.cs which is already in the shared location?

Copy link
Member Author

@TIHan TIHan Aug 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we directly reference BoundedMemory.cs and its implementations in Sve_r.csproj and Sve_ro.csproj, I do not have access to any other API in the TestUtilities.csproj nor do I have access to library internals; which means I cannot access Interop.Libraries.SystemNative.

@jkotas
Copy link
Member

jkotas commented Aug 1, 2024

The last CI run didn't have issues running the Unix implementation for BoundedMemory with the exception of Mono. This is due to using SystemNative from CoreClr; Mono doesn't have it. I added the check to ensure we do not execute the Unix impl on Mono.

SystemNative is not runtime specific. It is available when running on Mono too. We have number libraries that use it and that work on Mono just fine.

@TIHan
Copy link
Member Author

TIHan commented Aug 1, 2024

We have number libraries that use it and that work on Mono just fine.

Ok, that's good to know. I assumed it wouldn't be able to access it as I got an error calling into the library here: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-105595-merge-e7641fefa451458d81/Invariant.Tests/1/console.5fc11457.log?helixlogtype=result

@TIHan
Copy link
Member Author

TIHan commented Aug 1, 2024

It looks like the error is just wasm specific.

@jkotas
Copy link
Member

jkotas commented Aug 1, 2024

I assumed it wouldn't be able to access it as I got an error calling into the library here: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-105595-merge-e7641fefa451458d81/Invariant.Tests/1/console.5fc11457.log?helixlogtype=result

This is wasm build. This error says that mprotect is not available. Memory protections do not exist on wasm, so it makes sense that mprotect does not exist on wasm. I think you want to exclude this for wasm, and not mono.

@TIHan
Copy link
Member Author

TIHan commented Aug 1, 2024

I added the check for wasm, @jkotas does it look ok?

LIR::Use use;
bool foundUse = BlockRange().TryGetUse(node, &use);

if (m_ffrTrashed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this code adding the extra arg that is checked for in hwintrinsiccodegenarm64.cpp which causes the wrFFR ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, now I'm wondering if this code was left over from a merge.

Copy link
Member Author

@TIHan TIHan Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this was not left over. @kunalspathak added this for this PR.

When looking at hwintrinsiccodegenarm64.cpp, it does seem to be the case as it checks for the num of operands.

@@ -3658,6 +3658,118 @@ public new abstract class Arm64 : AdvSimd.Arm64
public static unsafe Vector<ulong> GatherVectorByteZeroExtend(Vector<ulong> mask, byte* address, Vector<ulong> indices) { throw new PlatformNotSupportedException(); }


/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the "// Unextended load, first-faulting" above this similar to the other APIs. In both API files. And for the other APIs you're adding too.

@@ -4842,6 +4842,26 @@ public new abstract partial class Arm64 : System.Runtime.Intrinsics.Arm.AdvSimd.
public static unsafe System.Numerics.Vector<ulong> GatherVectorByteZeroExtend(System.Numerics.Vector<ulong> mask, byte* address, System.Numerics.Vector<long> indices) { throw null; }
public static System.Numerics.Vector<ulong> GatherVectorByteZeroExtend(System.Numerics.Vector<ulong> mask, System.Numerics.Vector<ulong> addresses) { throw null; }
public static unsafe System.Numerics.Vector<ulong> GatherVectorByteZeroExtend(System.Numerics.Vector<ulong> mask, byte* address, System.Numerics.Vector<ulong> indices) { throw null; }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove any new blank lines from this file please.

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Aug 2, 2024
@TIHan
Copy link
Member Author

TIHan commented Aug 2, 2024

Merging. I carefully looked into each of the current failing CI legs and they are not related to the changes made here.
There are no more 'mprotect' errors; it's been resolved.

@TIHan TIHan merged commit df09fd1 into dotnet:main Aug 2, 2024
160 of 169 checks passed
@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Aug 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2024
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.

10 participants