Adding a DebugView for Vector64<T>, Vector128<T>, and Vector256<T> #15897
Conversation
|
FYI. @fiigii, @sdmaclea, @CarolEidt, @eerhardt, @RussKeldorph, @jkotas |
| { | ||
| get | ||
| { | ||
| // The IsPrimitive check ends up working for `bool`, `char`, `IntPtr`, and `UIntPtr` |
There was a problem hiding this comment.
As per the comment, this ends up working for bool, char, IntPtr, and UIntPtr at debug time, but makes the subsequent code much simpler.
Given that this only works under a debugger, and users still can't do anything useful with such a type, I think this should be fine.
| public struct Vector128<T> where T : struct | ||
| { | ||
| // These fields exist purely so debug view works (https://github.com/dotnet/coreclr/issues/15694) | ||
| private byte _00; |
There was a problem hiding this comment.
As per the comment, these exist so the debugger will properly marshal bytes. There are a couple bugs on GitHub tracking this and a couple internal ones as well.
There was a problem hiding this comment.
Do these byte fields affect how Vector128<T> fields are aligned?
There was a problem hiding this comment.
I don't think so. System.Numerics.Vector<T> also has byte fields (although public, and indirected through the Register struct).
Is there a good way of validating this today?
There was a problem hiding this comment.
Create struct like this and check the offsets of _b and _d:
struct MyStruct {
byte _a;
Vector<long> _b;
byte _c;
Vector<double> _d;
}It is possible that the alignment is not right already. I think the fields _b and _d should be aligned at 8 bytes in this case.
There was a problem hiding this comment.
Looks like the alignment is not right already.
I ran four tests (multiple, separate, process executions to ensure I got the correct results):
public struct MyStruct128 {
public byte _a;
public Vector128<long> _b;
public byte _c;
public Vector128<double> _d;
}
public static void Test128_1() {
Vector128<ulong> val;
Console.Write("Vector128 is ");
TestAlignment(Unsafe.AsPointer(ref val));
}
public static void Test128_2() {
byte _a;
Vector128<ulong> val;
Console.Write("Vector128 is ");
TestAlignment(Unsafe.AsPointer(ref val));
}
public static void Test128_3() {
MyStruct128 val;
Console.Write("Vector128 is ");
TestAlignment(Unsafe.AsPointer(ref val._b));
}
public static void Test128_4() {
MyStruct128 val;
Console.Write("Vector128 is ");
TestAlignment(Unsafe.AsPointer(ref val._d));
}I saw the following on x64:
- Test128_1: val is 16-byte aligned
- Test128_2: val is 16-byte aligned
- Test128_3: val._b is 1-byte aligned
- Test128_3: val._b is 2-byte aligned
I saw the following on x86:
- Test128_1: val is 4-byte aligned
- Test128_2: val is 8-byte aligned
- Test128_3: val._b is 1-byte aligned
- Test128_3: val._b is 2-byte aligned
There was a problem hiding this comment.
I think, based on the previous discussions, it would need to be only for stack-based allocations (modifying the GC to support arbitrary alignments is hard).
However, that might be confusing and might also be non-trivial to achieve.
There was a problem hiding this comment.
I was referring the layout of structs containing vectors. In C/C++ the following struct
struct foo {
char b;
__m256i v;
};is 64 byte in size, there's 31 bytes of padding between b and v.
As is now, the only way to reproduce such a layout in .NET is to use explicit struct layout.
The fact that such a struct is not guaranteed to be 32 byte aligned when placed in the GC heap is a separate issue.
There was a problem hiding this comment.
Are we sure all platforms will have the same layout requirements?
the only way to reproduce such a layout in .NET is to use explicit struct layout.
Is that a bad thing?
There was a problem hiding this comment.
We don't currently have runtime support for 16/32 byte alignment. @jkotas can probably chime in on how difficult it would be to add.
Non-trivial non-pay-for-play GC feature. Related discussion:
https://github.com/dotnet/coreclr/issues/5195
https://github.com/dotnet/corefx/issues/22790
There was a problem hiding this comment.
Are we sure all platforms will have the same layout requirements?
Good question. I suppose you may know how this works on ARM? It's probably even more interesting if you drag in ARM's SVE.
Is that a bad thing?
Hard to say. It may be problematic if it turns out that different platforms use different alignments. For example, if ARM's 32 byte vectors are only 16 byte aligned then my foo struct will have only 15 bytes of padding. Then you'll have to create different .NET structs for ARM and x86 because the explicit struct layout that you can define in .NET using attributes is practically hardcoded at compile time.
|
|
||
| namespace System.Runtime.Intrinsics | ||
| { | ||
| public struct Vector128DebugView<T> where T : struct |
There was a problem hiding this comment.
This should be internal class. (To match what all other debug views look like.)
There was a problem hiding this comment.
Oops. Changed this when I was testing the debug marshalling issue, forgot to revert.
|
|
||
| if (typeof(T).IsPrimitive) | ||
| { | ||
| var items = new T[16 / Marshal.SizeOf<T>()]; |
There was a problem hiding this comment.
Why not Unsafe.SizeOf<T>? Marshal.SizeOf has a different opinion about the size of bool so the line below will just do an unnecessary memory corruption, should the user use this with a pointless type.
| } | ||
| } | ||
|
|
||
| [CLSCompliant(false)] |
There was a problem hiding this comment.
Nit: These CLSCompliant attributes should not be necessary now that the type is internal.
6 participants
This implements https://github.com/dotnet/coreclr/issues/15694