-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[Arm64] Implement managed Short Vector ABI for Vector64<T> Vector128<T> #16310
Conversation
Is it "accurate" to treat My understanding of the spec is that Additionally, I had thought that any type which is composed of short vectors would generally make it a |
Correct.
Correct. To be semantically correct, I should change all references to HFA to HomogeneousAggregate (or any other generic name). It didn't seem necessary or worth it, but it could easily be done with a global search and replace. If it was renamed, it should be done in a separate PR. SVE will introduce two new vector types and two new aggregate types.
It seemed silly to treat these all differently, when the only difference would be the aggregated type.
It is accurate to treat It also could be accurate to treat In the JIT we treat a single element homogenous aggregate as the primitive type ( Observation:
This is all logical because the Homogenous Aggregates rules are based on in memory layout. Struct is a higher level language construct which doesn't affect the ABI. |
Looking at the spec, I think the primary benefit in differentiating is that there are a number of steps which differ slightly for a fundamental type vs a HVA/HFA type that can ultimately make the argument passing logic simpler for a given type. That being said, I think that can probably be tracked by a work item and fixed/handled later (But I do think it will be worth fixing, eventually). |
Can you be more specific? I do not see a functional difference. JIT already treated |
OK, maybe |
While it does look to be the case that an HVA containing a single |
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 looked through all the changes and
generally they look good to me
src/inc/corhdr.h
Outdated
@@ -913,6 +913,9 @@ typedef enum CorElementType | |||
ELEMENT_TYPE_SENTINEL = 0x01 | ELEMENT_TYPE_MODIFIER, // sentinel for varargs | |||
ELEMENT_TYPE_PINNED = 0x05 | ELEMENT_TYPE_MODIFIER, | |||
|
|||
ELEMENT_TYPE_SHORT_VECTOR = 0x80, |
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 file is meant to contain only the standard ELEMENT_TYPE_
constants that show up in regular metadata.
The various custom area specific should rather be somewhere else. corpriv.h may be a good place - there are several of them already (look for ELEMENT_TYPE_
).
@@ -1664,6 +1664,31 @@ CorElementType MethodTable::GetHFAType() | |||
_ASSERTE(pMT->IsValueType()); |
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 you need to modify all callers of GetHFAType to handle these new values correctly?
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 thought I looked at them all,but maybe I maybe I waiting for an assert to fire... I'll take a look next week.
For Arm64 C#, these are treated the same as
I looked at those operations as I can believe I missed something. When we look at native interoperability, we may find something important. |
@dotnet-bot help |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
d01a77f
to
5001bae
Compare
Fixed broken test, rebased, fixed merge conflicts, and enabled simd tests now that Arm64 HW intrinsics are included in corefx packages. |
ea14dcf
to
eaa6e07
Compare
eaa6e07
to
ab00d8c
Compare
This PR got a bit crufty while I was working on enabling official arm64 builds. This is important for arm64 HW intrinsics ZBB. I will work on fixing this. |
@dotnet-bot help |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
This change implements changes necessary to support
Vector64<T>
Vector128<T>
as Homogeneous Vector Aggregate (HVA
) types each containing a singleShort Vector
.It should also implement most of the change necessary to also handle multiple element
HVA
in managed code. I actually believe the functional implementation is complete. Lacking test coverage it is difficult to confirm this also works. I will write a test shortly.This change is fairly extensive. I would appreciate an early review.
Current status:
LinearScan::checkLastUses()
(debugging now)#ifdef
.FEATURE_ARM64_HVA_ABI
#ifdef
in most places even when it felt like the code should be commonFixes #14371
Fixes #16021
Prepares to fix #16022
@janvorli @jkotas @RussKeldorph @CarolEidt @tannergooding PTAL
@dotnet/jit-contrib @dotnet/arm64-contrib FYI