-
Notifications
You must be signed in to change notification settings - Fork 5k
Improve support for 512-bit Vector<T> #111472
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @tannergooding. This is ready for review.
Would it be all right to add the alt-sized Vector<T>
configs to one of the jitstress-isas
pipelines?
@@ -17,6 +17,6 @@ | |||
</PropertyGroup> | |||
|
|||
<ItemGroup> | |||
<Compile Include="CpuId.cs" /> | |||
<Compile Include="../../../JIT/HardwareIntrinsics/X86/X86Base/CpuId.cs" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local copy of these tests had been bit rotting for a while. I could bring it up to date, but it seems reasonable to just share it with JIT.
succeeded &= VectorByteCount == 32; | ||
// MaxVectorTBitWidth env variable can be used to change Vector<T> size. | ||
// We can only assume it is at least 16 bytes. | ||
succeeded &= VectorByteCount >= 16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept changes to the regression tests minimal -- this is handled more robustly in the CpuId tests.
/azp run runtime-coreclr jitstress-isas-x86 |
Azure Pipelines successfully started running 1 pipeline(s). |
We do not guarantee alignment of writes in |
That's an issue with the code using It's not a blocker for this PR which enables |
@saucecontrol Are you waiting for review, or are you still working on this? @tannergooding You've looked at this before; can you look again? |
(@saucecontrol you should probably merge up to main) |
It's ready on my end. Never did get a clean CI run, but no failures appeared to be related. Let's see how it does after merging back to main. |
if (Vector<uint>.Count == 8) | ||
if (Vector<uint>.Count == 16) | ||
{ | ||
// TODO: P/Invoke tests do not yet handle 512-bit Vector<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to either be handled or have a tracking issue that is picked up shortly after merge in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worded that comment oddly. The issue is not specific to Vector<T>
but rather Vector512
in general isn't covered by these interop tests. Do we have something tracking that already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't believe so. These tests are really centered around Vector and most of the V128/256/512 tests are elsewhere and newer/different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are under tests/Interop/PInvoke/Generics, which AFAICT cover scenarios involving all the generic types that have specific ABI handling. What I mean is that we don't currently have any tests for Vector512
ABI handling. Do we even have support for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even have support for that?
Yesn't. It's expected to throw today, but the tests are still setup since there's only a single known issue actually blocking the support (which is that the default calling convention for returning vectors on Windows should be in register, not via a return buffer).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests are still setup
This is the part I'm confused about. Vector512
is conspicuously absent from the list of types covered by the tests under https://github.com/dotnet/runtime/tree/main/src/tests/Interop/PInvoke/Generics.
I'm happy to open a tracking issue for adding them, but I would have assumed that would be an implicit part of enabling the ABI support.
I'm now concerned we may be missing some coverage, though. If Vector512
used in P/Invoke is supposed to throw, we'd need to be sure the same happens for 512-bit Vector<T>
, but I don't see where we're testing either.
CC. @dotnet/jit-contrib, @EgorBo, @BruceForstall for secondary review |
Fixes #104978
Contributes to #108869
Vector<T>
or assumed AVX2 support meantVector<T>
was 256-bit--max-vectort-bitwidth:512
in AOT and adds a smoke test for itVector<T>
is no larger thanPreferredVectorBitWidth
Tests pass locally (AVX-512 machine) with
DOTNET_MaxVectorTBitWidth=512
andDOTNET_MaxVectorTBitWidth=128
I believe this also addresses any problems that prevented opportunistic enablement of AVX2 on AOT when AVX is enabled, but I'll do some more testing around that and make a separate PR for that change if all is well.