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 ARM64-SVE: Add Count*BitElements #101188

Merged
merged 9 commits into from
Apr 26, 2024

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented Apr 17, 2024

Moves HW_Category_EnumPattern to a flag, as these APIs need both EnumPattern and HW_Category_Scalar.

Test results:

❯ $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_Count
12:43:02.239 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count16BitElements()
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario_UnsafeRead
Beginning scenario: RunReflectionScenario_UnsafeRead
Beginning scenario: RunLclVarScenario_UnsafeRead
Beginning scenario: RunClassFldScenario
Beginning scenario: RunStructLclFldScenario
Beginning scenario: RunStructFldScenario
12:43:02.340 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count16BitElements()
12:43:02.347 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count32BitElements()
Beginning scenario: RunBasicScenario_UnsafeRead
Beginning scenario: RunReflectionScenario_UnsafeRead
Beginning scenario: RunLclVarScenario_UnsafeRead
Beginning scenario: RunClassFldScenario
Beginning scenario: RunStructLclFldScenario
Beginning scenario: RunStructFldScenario
12:43:02.354 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count32BitElements()
12:43:02.356 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count64BitElements()
Beginning scenario: RunBasicScenario_UnsafeRead
Beginning scenario: RunReflectionScenario_UnsafeRead
Beginning scenario: RunLclVarScenario_UnsafeRead
Beginning scenario: RunClassFldScenario
Beginning scenario: RunStructLclFldScenario
Beginning scenario: RunStructFldScenario
12:43:02.363 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count64BitElements()
12:43:02.365 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count8BitElements()
Beginning scenario: RunBasicScenario_UnsafeRead
Beginning scenario: RunReflectionScenario_UnsafeRead
Beginning scenario: RunLclVarScenario_UnsafeRead
Beginning scenario: RunClassFldScenario
Beginning scenario: RunStructLclFldScenario
Beginning scenario: RunStructFldScenario
12:43:02.372 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_Count8BitElements()

@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 17, 2024
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.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 17, 2024
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@a74nh a74nh marked this pull request as ready for review April 17, 2024 16:07
@a74nh
Copy link
Contributor Author

a74nh commented Apr 17, 2024

@kunalspathak @dotnet/arm64-contrib

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 questions.

@@ -2887,6 +2887,10 @@

(string templateFileName, Dictionary<string, string> templateData)[] SveInputs = new []
{
("ScalarUnOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_Count16BitElements", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "Count16BitElements", ["RetBaseType"] = "UInt64", ["Op1BaseType"] = "SveMaskPattern", ["LargestVectorSize"] = "8", ["NextValueOp1"] = "SveMaskPattern.All", ["ValidateResult"] = "isUnexpectedResult = (result != (UInt64)(Unsafe.SizeOf<Vector<Int16>>() / sizeof(Int16)));",}),
Copy link
Member

Choose a reason for hiding this comment

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

From #100606 (comment), LargestVectorSize should be 256. You can fix it in a follow-up PR.

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

…sses.

TEST_LABEL: ent-arch-aarch64
TEST_IMG: ubuntu/dotnet-build
TEST_CMD: safe ./projects/dotnet/test-runtime.sh --scope coreclr,libs

Jira: ENTLLT-7328

Change-Id: I727edf8652a5c8648e7008d4ca47e7a4f36d5a1e
Comment on lines 193 to 194
// The intrinsic has an enum operand. Using this implies HW_Flag_HasImmediateOperand.
HW_Flag_HasEnumOperand = 0x80000,
Copy link
Member

@tannergooding tannergooding Apr 23, 2024

Choose a reason for hiding this comment

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

It's not clear to me why we need this in addition to HasImmediateOperand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me why we need this in addition to HasImmediateOperand?

This is just for the hwintrinsiccodegen - so that it can pick the correct emitIns_ function to call (namely emitIns_R_PATTERN).

Although, given an immediate and enum are effectively the same, I wonder if we should eventually do some refactoring so that it goes via emitIns_R_I() using an int and then remove emitIns_R_PATTERN().

@a74nh
Copy link
Contributor Author

a74nh commented Apr 25, 2024

Stress test results:

❯ ~/dotnet/sve_api_runtime/stress_tester.py $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_Count
===================Running default===================
{}
===================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'}

@JulieLeeMSFT
Copy link
Member

Stress test results:

❯ ~/dotnet/sve_api_runtime/stress_tester.py $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_Count
===================Running default===================
{}
===================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'}

@a74nh, what does this output mean like 0, 1, 2,...?

@kunalspathak
Copy link
Member

@a74nh, what does this output mean like 0, 1, 2,...?

@JulieLeeMSFT - those are the environment variables value we are setting before executing all the test cases. This is to future proof ourselves until we make SVE machines in CI operational and run the stress legs.

@kunalspathak
Copy link
Member

@a74nh - you want to revert 2937557 before merging?

@kunalspathak kunalspathak self-requested a review April 25, 2024 17:27
@a74nh
Copy link
Contributor Author

a74nh commented Apr 25, 2024

@a74nh - you want to revert 2937557 before merging?

Done

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

@kunalspathak kunalspathak merged commit 4c14fb0 into dotnet:main Apr 26, 2024
163 of 168 checks passed
@a74nh a74nh deleted the sve_CountElements_github branch April 29, 2024 07:45
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* JIT ARM64-SVE: Add Count*BitElements

* Generic ValidateResult testing

* Fix formatting

* Add option to change SVE vector length for current and children processes.

TEST_LABEL: ent-arch-aarch64
TEST_IMG: ubuntu/dotnet-build
TEST_CMD: safe ./projects/dotnet/test-runtime.sh --scope coreclr,libs

Jira: ENTLLT-7328

Change-Id: I727edf8652a5c8648e7008d4ca47e7a4f36d5a1e

* Revert "Add option to change SVE vector length for current and children processes."

This reverts commit 2937557.

* fix CreateTrueMask

---------

Co-authored-by: Swapnil Gaikwad <swapnil.gaikwad@arm.com>
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* JIT ARM64-SVE: Add Count*BitElements

* Generic ValidateResult testing

* Fix formatting

* Add option to change SVE vector length for current and children processes.

TEST_LABEL: ent-arch-aarch64
TEST_IMG: ubuntu/dotnet-build
TEST_CMD: safe ./projects/dotnet/test-runtime.sh --scope coreclr,libs

Jira: ENTLLT-7328

Change-Id: I727edf8652a5c8648e7008d4ca47e7a4f36d5a1e

* Revert "Add option to change SVE vector length for current and children processes."

This reverts commit 2937557.

* fix CreateTrueMask

---------

Co-authored-by: Swapnil Gaikwad <swapnil.gaikwad@arm.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI arm-sve Work related to arm64 SVE/SVE2 support community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants