Adding the Vector64<T> type to the S.R.Intrinsics assembly#26432
Conversation
| namespace System.Runtime.Intrinsics | ||
| { | ||
| [StructLayout(LayoutKind.Sequential, Size = 8)] | ||
| public struct Vector64<T> where T : struct {} |
There was a problem hiding this comment.
Are we including private fields in the reference assemblies now? I think I remember some thread on the topic...
Asking since we added private fields here: dotnet/coreclr#15897
There was a problem hiding this comment.
Also not clear on if we need the [Intrinsic] attribute (which was already missing) or if we need the Debugger* attributes, which are new.
There was a problem hiding this comment.
Are we including private fields in the reference assemblies now? I think I remember some thread on the topic...
Yes. See the following for more details.
dotnet/buildtools#1859
#26286
http://blog.paranoidcoding.com/2016/02/15/are-private-members-api-surface.html
There was a problem hiding this comment.
For the DebuggerTypeProxy attributes, I don't see any others in a ref file. So I'd assume we don't need them in the ref assembly. Plus, since it points to a type that is internal, it can't be. And lastly, the debugger doesn't know anything about 'ref' assemblies.
The [Intrinsic] attribute is internal, right? So it can't be in the ref assembly.
|
This was already added to corlib, but hasn't been added here yet. |
| public struct Vector256<T> where T : struct {} | ||
| } | ||
|
|
||
| namespace System.Runtime.Intrinsics.X86 |
There was a problem hiding this comment.
Might be good to have one file per namespace
I was adding ARM64 intrinsics to a separate file
There was a problem hiding this comment.
Might be good to have one file per namespace
I agree. However, it doesn't appear any of the other reference sources do this...
There was a problem hiding this comment.
I was wondering the same thing. My initial thought was that the others are in a single file because they are generated by a tool. But maybe we consciously made the decision to have a single file for the whole ref assembly.
@stephentoub @weshaggard @ericstj @KrzysztofCwalina - thoughts/opinions?
There was a problem hiding this comment.
Personally, I find the current ref C# files hard to read. There are no line breaks between namespaces/classes and no breaks between things like properties vs methods/etc.
Even if being in a single file is a conscious decision, it might be worthwhile changing the format slightly to make it more readable.
|
I didn't create PR for corefx, since none of the ARM64 intrinsic API's have been approved. |
|
If you want them, I have a corefx patch for all the open API Proposals. I have been using it for testing. I wasn't going to merge until API was supported on CoreCLR and API was approved. |
Ah, I had thought this one in particular was approved. I had just discovered that the API was missing when testing out the |
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
I think this looks good. The other questions can be resolved in a follow up, if necessary.
| namespace System.Runtime.Intrinsics | ||
| { | ||
| [StructLayout(LayoutKind.Sequential, Size = 8)] | ||
| public struct Vector64<T> where T : struct {} |
There was a problem hiding this comment.
Are we including private fields in the reference assemblies now? I think I remember some thread on the topic...
Yes. See the following for more details.
dotnet/buildtools#1859
#26286
http://blog.paranoidcoding.com/2016/02/15/are-private-members-api-surface.html
| namespace System.Runtime.Intrinsics | ||
| { | ||
| [StructLayout(LayoutKind.Sequential, Size = 8)] | ||
| public struct Vector64<T> where T : struct {} |
There was a problem hiding this comment.
For the DebuggerTypeProxy attributes, I don't see any others in a ref file. So I'd assume we don't need them in the ref assembly. Plus, since it points to a type that is internal, it can't be. And lastly, the debugger doesn't know anything about 'ref' assemblies.
The [Intrinsic] attribute is internal, right? So it can't be in the ref assembly.
@eerhardt, going to add the private fields. Will merge after that passes all the CI checks. |
You are going to add them to all the Vector structs, right? If so, that plan sounds good to me. |
|
Updated to include a dummy private field, as per the convention in dotnet/buildtools#1859 |
|
Jobs are all hung trying to push XUnit results. CC. @mmitche |
|
cc @dotnet/dnceng. @MattGal sent out a notice about some processing delays a few mins ago. |
|
test Linux x64 Release Build please |
|
@tannergooding we understand the issue and work processing should be proceeding at normal pace now, so please kick off all you'd like. |
6 participants
FYI. @sdmaclea, @eerhardt