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 Sve.CreateFalseMask*() #102076

Merged
merged 6 commits into from
May 13, 2024

Conversation

mikabl-arm
Copy link
Contributor

@mikabl-arm mikabl-arm commented May 10, 2024

Also added tests for Sve.CreateTrueMask*()

Tests results:

~/dotnet/runtime$ $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_CreateFalse
12:37:26.447 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskByte_byte()
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario
12:37:26.585 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskByte_byte()
12:37:26.603 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskDouble_double()
Beginning scenario: RunBasicScenario
12:37:26.618 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskDouble_double()
12:37:26.622 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskInt16_short()
Beginning scenario: RunBasicScenario
12:37:26.629 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskInt16_short()
12:37:26.644 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskInt32_int()
Beginning scenario: RunBasicScenario
12:37:26.650 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskInt32_int()
12:37:26.654 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskInt64_long()
Beginning scenario: RunBasicScenario
12:37:26.673 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskInt64_long()
12:37:26.677 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskSByte_sbyte()
Beginning scenario: RunBasicScenario
12:37:26.684 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskSByte_sbyte()
12:37:26.688 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskSingle_float()
Beginning scenario: RunBasicScenario
12:37:26.705 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskSingle_float()
12:37:26.709 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskUInt16_ushort()
Beginning scenario: RunBasicScenario
12:37:26.716 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskUInt16_ushort()
12:37:26.720 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskUInt32_uint()
Beginning scenario: RunBasicScenario
12:37:26.741 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskUInt32_uint()
12:37:26.745 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskUInt64_ulong()
Beginning scenario: RunBasicScenario
12:37:26.752 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateFalseMaskUInt64_ulong()
~/dotnet/runtime$ $CORE_ROOT/corerun ./artifacts/tests/coreclr/linux.arm64.Checked/JIT/HardwareIntrinsics/HardwareIntrinsics_Arm_ro/HardwareIntrinsics_Arm_ro.dll Sve_CreateTrue
12:36:41.326 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskByte()
Supported ISAs:
  AdvSimd:   True
  Aes:       True
  ArmBase:   True
  Crc32:     True
  Dp:        True
  Rdm:       True
  Sha1:      True
  Sha256:    True
  Sve:       True

Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.412 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskByte()
12:36:41.421 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskDouble()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.431 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskDouble()
12:36:41.435 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskInt16()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.459 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskInt16()
12:36:41.463 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskInt32()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.473 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskInt32()
12:36:41.477 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskInt64()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.487 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskInt64()
12:36:41.491 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskSByte()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.501 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskSByte()
12:36:41.520 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskSingle()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.530 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskSingle()
12:36:41.534 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskUInt16()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.544 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskUInt16()
12:36:41.554 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskUInt32()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.562 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskUInt32()
12:36:41.569 Running test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskUInt64()
Beginning scenario: RunBasicScenario_CreateTrueMask
12:36:41.596 Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_CreateTrueMaskUInt64()

Also added tests for Sve.CreateTrueMask*()
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 May 10, 2024
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.

@mikabl-arm
Copy link
Contributor Author

@kunalspathak @dotnet/arm64-contrib @a74nh

@@ -665,6 +665,96 @@ public new abstract class Arm64 : AdvSimd.Arm64
public static unsafe ulong Count8BitElements([ConstantExpected] SveMaskPattern pattern = SveMaskPattern.All) => Count8BitElements(pattern);


/// CreateFalseMaskByte : Set all predicate elements to false
Copy link
Member

Choose a reason for hiding this comment

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

nit: /// is the XML documentation comment prefix and so needs to follow the standard doc comment prefix, being in the <summary> region if it's present.

It looks like this ended up being missed in past reviews, but it may cause problems with the downstream tools and will likely require the documentation team to do more work if it's not fixed.

We should only need the Set all predicate elements to false part of the text since the method name is implicit from the member the comment is attached to.

Copy link
Member

@kunalspathak kunalspathak May 10, 2024

Choose a reason for hiding this comment

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

It looks like this ended up being missed in past reviews

good catch. will fix it for other docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I change it to the following? If not, please provide an example based on this comment.

        // Set all predicate elements to false                                                                                                                                                                                                                                                                              

        /// <summary>                                                                                                                                                                                                                                                                                                       
        /// svbool_t svpfalse[_b]()                                                                                                                                                                                                                                                                                         
        ///   PFALSE Presult.B                                                                                                                                                                                                                                                                                              
        /// </summary>                                                                                                                                                                                                                                                                                                      
        public static unsafe Vector<byte> CreateFalseMaskByte() => CreateFalseMaskByte();

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that looks right
(I'd prefer to keep the CreateFalseMaskByte, because then it's explicit what the comment attaches to, but let's keep with convention)

Copy link
Member

Choose a reason for hiding this comment

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

should be /// instead of // and that basically is representing the logical group name of APIs that follows the comments.

- // Set all predicate elements to false
+ /// Set all predicate elements to false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please check.

Copy link
Member

@tannergooding tannergooding left a comment

Choose a reason for hiding this comment

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

LGTM minus the XML doc comment fix that's needed.

@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label May 10, 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.

Changes looks good. Added some comments around test.

@@ -0,0 +1,117 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not named as SveCreateFalseMaskTest.template?

Copy link
Member

Choose a reason for hiding this comment

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

Are you planning to reuse it for other APIs?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was my suggestion - I'd hope we can use this one for future APIs. Whereas the SveCreateTrueMaskTest.template is specific to true mask due to the handling.

@@ -3009,6 +3009,27 @@
("SveVecBinOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_Divide_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "Divide", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["ValidateIterResult"] = "Helpers.Divide(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.Divide(left[i], right[i])"}),
("SveVecBinOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_Divide_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "Divide", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Double", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetDouble()", ["NextValueOp2"] = "TestLibrary.Generator.GetDouble()", ["ValidateIterResult"] = "Helpers.Divide(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.Divide(left[i], right[i])"}),

("SveSimpleNoOpTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_CreateFalseMaskByte_byte", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "CreateFalseMaskByte", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Byte", ["LargestVectorSize"] = "8", ["ValidateIterResult"] = "result[i] != 0",}),
Copy link
Member

Choose a reason for hiding this comment

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

["LargestVectorSize"] = "64" for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's not used by SveSimpleNoOpTest.template and can be safely removed along with an according line in the test template. Sounds good?

Copy link
Member

Choose a reason for hiding this comment

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

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, please check.

throw new Exception("Incorrect esize");
}

int elements = (int)(Sve.Count64BitElements()) * 64 / esize;
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
int elements = (int)(Sve.Count64BitElements()) * 64 / esize;
int elements = (int)(Sve.Count8BitElements() / esize);

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 would have to be int elements = (int)(Sve.Count8BitElements()) * 8 / esize; anyway I suppose? Or do you suggest to make esize denote the size of elements in bytes as well? At the moment it's in bits, not bytes.

Copy link
Member

Choose a reason for hiding this comment

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

yes, should have * 8 and that gives better readability in terms of "X bytes".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

test.RunBasicScenario_CreateFalseMask();

// Validates calling via reflection works
// TODO-SVE: Enable once register allocation exists for predicates.
Copy link
Member

Choose a reason for hiding this comment

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

@a74nh - can you remind me why this doesn't work without predicate register support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from another template without giving it a critical look. Will be happy to remove if @a74nh confirms that the comment isn't required here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I enabled this test for SveLoadVector_int and ran it. It passed. Not sure what the issues was now - was possibly due to missing mask handling (not predicates).

@mikabl-arm - could you uncomment this and remove the TODO, it should work.

We should re-enable all the others in a future PR.


// DecodePredCount()
// =================
// integer DecodePredCount(bits(5) bitpattern, integer esize)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we can have the "copy-pasted" code of this from Arm manual in dotnet codebase because of copyrights. can we simplify the summary docs of DecodePredCount instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// bits(PL) result;
// constant integer psize = esize DIV 8;
//
// for e = 0 to elements-1
Copy link
Member

Choose a reason for hiding this comment

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

same here. Please simplify and write the description in text of what we are trying to accomplish.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kunalspathak
Copy link
Member

Tests results:

Can you also please run the stress test on these? For more details on how to run them, please refer the notes at
#99957 (comment) under "Stress testing" section.

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!

@@ -665,6 +665,96 @@ public new abstract class Arm64 : AdvSimd.Arm64
public static unsafe ulong Count8BitElements([ConstantExpected] SveMaskPattern pattern = SveMaskPattern.All) => Count8BitElements(pattern);


/// Set all predicate elements to false
Copy link
Member

Choose a reason for hiding this comment

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

nit: This should really be written just once, just as we do for e.g. in /// BitwiseClear : Bitwise clear or /// BooleanNot : Logically invert boolean condition. Same applies for CreateTrue* APIs. @mikabl-arm - do you mind fixing it in your next PR please?

@@ -608,6 +608,96 @@ public new abstract class Arm64 : AdvSimd.Arm64
public static unsafe ulong Count8BitElements([ConstantExpected] SveMaskPattern pattern = SveMaskPattern.All) { throw new PlatformNotSupportedException(); }


/// Set all predicate elements to false
Copy link
Member

Choose a reason for hiding this comment

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

same here.

@kunalspathak kunalspathak merged commit bed87f8 into dotnet:main May 13, 2024
164 of 167 checks passed
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
* JIT ARM64-SVE: Add Sve.CreateFalseMask*()

Also added tests for Sve.CreateTrueMask*()

* Remove code copied from the Arm manual.

* Remove LargestVectorSize from CreateFalseMask* template parameters

* Fix documentation comments for CreateFalseMask*()

* Improve code readability by operating byte-sized elements.

* Enable reflection scenario tests
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics 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.

4 participants