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 Prefetch* APIs. #103094

Merged
merged 20 commits into from
Jun 12, 2024
Merged

JIT: Added SVE Prefetch* APIs. #103094

merged 20 commits into from
Jun 12, 2024

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Jun 5, 2024

Contributes to #99957

Adds:

  • PrefetchBytes
  • PrefetchInt16
  • PrefetchInt32
  • PrefetchInt64

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@@ -4940,6 +4945,22 @@ public enum SveMaskPattern : byte
LargestMultipleOf3 = 30, // The largest multiple of 3.
All = 31 // All available (implicitly a multiple of two).
};

public enum SvePrefetchType : byte
Copy link
Contributor Author

@TIHan TIHan Jun 5, 2024

Choose a reason for hiding this comment

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

@kunalspathak @tannergooding was this enum type approved? I ask because of the SV_PLDL1KEEP names.

Copy link
Member

Choose a reason for hiding this comment

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

I see a note here about they not approved? @tannergooding can you confirm?

#94006 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

#97831 approved the Prefetch* APIs.

The enum should have been approved as part of that, but wasn't looked at directly.

Copy link
Member

Choose a reason for hiding this comment

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

The enum names are different and were approved in #94007 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@TIHan - can you update the names accordingly?

@TIHan
Copy link
Contributor Author

TIHan commented Jun 5, 2024

@dotnet/arm64-contrib @kunalspathak this is ready. There is no visible output to validate since this instruction is intended to be a hint.

===================Running default===================
------------------- {} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_0() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_1() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_2() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_3() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_4() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_5() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_6() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_7() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_8() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_9() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_10() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchBytes_11() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchInt16() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchInt32() : 3
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_PrefetchInt64() : 3
===================Running jitstress===================
------------------- {'JitMinOpts': '1'} -------------------
------------------- {'JitStress': '1'} -------------------
------------------- {'JitStress': '2'} -------------------
------------------- {'JitStress': '1', 'TieredCompilation': '1'} -------------------
------------------- {'JitStress': '2', 'TieredCompilation': '1'} -------------------
------------------- {'TailcallStress': '1'} -------------------
------------------- {'ReadyToRun': '0'} -------------------
===================Running jitstressregs===================
------------------- {'JitStressRegs': '1'} -------------------
------------------- {'JitStressRegs': '2'} -------------------
------------------- {'JitStressRegs': '3'} -------------------
------------------- {'JitStressRegs': '4'} -------------------
------------------- {'JitStressRegs': '8'} -------------------
------------------- {'JitStressRegs': '0x10'} -------------------
------------------- {'JitStressRegs': '0x80'} -------------------
------------------- {'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStressRegs': '0x2000'} -------------------
===================Running jitstress2-jitstressregs===================
------------------- {'JitStress': '2', 'JitStressRegs': '1'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '2'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '3'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '4'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '8'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x10'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x80'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x1000'} -------------------
------------------- {'JitStress': '2', 'JitStressRegs': '0x2000'} -------------------

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label Jun 5, 2024
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

added some comments.

@@ -4940,6 +4945,22 @@ public enum SveMaskPattern : byte
LargestMultipleOf3 = 30, // The largest multiple of 3.
All = 31 // All available (implicitly a multiple of two).
};

public enum SvePrefetchType : byte
Copy link
Member

Choose a reason for hiding this comment

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

I see a note here about they not approved? @tannergooding can you confirm?

#94006 (comment)

@kunalspathak
Copy link
Member

also, need to handle immediate value and table generation logic.

@kunalspathak
Copy link
Member

@a74nh - so for this API, there is no visual side effect, so wondering how do we test it? also, values 6 and 7 seems invalid(?) but we are not sure how to validate it.

@a74nh
Copy link
Contributor

a74nh commented Jun 6, 2024

@a74nh - so for this API, there is no visual side effect, so wondering how do we test it?

I'm not sure if there is anything we can test for. We can't read the setting back from the system.

also, values 6 and 7 seems invalid(?) but we are not sure how to validate it.

We can't pass 6 or 7 as an immediate as C# compile will fail. So this only applies where it's in a variable, and we generate the lookup table.

Testing this in C++ asm, prfw with 6 or 7 doesn't cause an error when run. So, I don't think we should do anything special, and keep this patch as is.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 6, 2024

@a74nh Are 6, 7, 14 and 15 valid constants? In the hwintrinsic codegen sve tests, I do test those constants and they will be decoded as you expect.

See the original PR: #98468

@a74nh
Copy link
Contributor

a74nh commented Jun 7, 2024

@a74nh Are 6, 7, 14 and 15 valid constants? In the hwintrinsic codegen sve tests, I do test those constants and they will be decoded as you expect.

See the original PR: #98468

Depends how you define valid :)
They don't do anything when used but they shouldn't cause an error. I think the code in #98468 is ok as it is.

@TIHan
Copy link
Contributor Author

TIHan commented Jun 11, 2024

@kunalspathak this is ready again.

#103288 will need to be merged first before this one.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

There is some left over code that I don't think is needed. Also we should generalize the test template.

src/coreclr/jit/fgdiagnostic.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/gentree.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/hwintrinsiclistarm64sve.h Outdated Show resolved Hide resolved
src/coreclr/jit/hwintrinsiclistarm64sve.h Outdated Show resolved Hide resolved
src/coreclr/jit/lsraarm64.cpp Show resolved Hide resolved
Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@TIHan
Copy link
Contributor Author

TIHan commented Jun 12, 2024

Woo!

@TIHan TIHan merged commit be2827c into dotnet:main Jun 12, 2024
163 of 167 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 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.

4 participants