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

Arm64/SVE: Implemented ConvertToint64 and ConvertToUInt64 #104069

Merged
merged 45 commits into from
Jun 29, 2024

Conversation

ebepho
Copy link
Contributor

@ebepho ebepho commented Jun 27, 2024

Contributes to #99957

ConvertToInt64 and ConvertToUInt64 stress test output:

Passed test: _Sve_r::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToInt64_long_float() : 13
Passed test: _Sve_r::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToInt64_long_double() : 13
Passed test: _Sve_r::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToUInt64_ulong_float() : 13
Passed test: _Sve_r::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToUInt64_ulong_double() : 13
===================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'} -------------------
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToInt64_long_float() : 13
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToInt64_long_double() : 13
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToUInt64_ulong_float() : 13
Passed test: _Sve_ro::JIT.HardwareIntrinsics.Arm._Sve.Program.Sve_ConvertToUInt64_ulong_double() : 13
===================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'} -------------------

cc @dotnet/arm64-contrib

ebepho and others added 30 commits June 5, 2024 14:30
updating branch due to assert issue.
@ebepho ebepho added the arm-sve Work related to arm64 SVE/SVE2 support label Jun 27, 2024
@@ -1731,6 +1731,8 @@ GenTree* Compiler::impHWIntrinsic(NamedIntrinsic intrinsic,
{
case NI_Sve_ConvertToInt32:
case NI_Sve_ConvertToUInt32:
case NI_Sve_ConvertToInt64:
Copy link
Member

Choose a reason for hiding this comment

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

I expected to have entries for ConvertToSingle and ConvertToDouble here as well. Did you confirm this works for scenario where maskSize != operSize in lowering?

(embOp->GetHWIntrinsicId() == NI_Sve_ConvertToUInt32));
assert((embOp->GetHWIntrinsicId() == NI_Sve_ConvertToInt32) ||
(embOp->GetHWIntrinsicId() == NI_Sve_ConvertToUInt32) ||
(embOp->GetHWIntrinsicId() == NI_Sve_ConvertToInt64) ||
Copy link
Member

Choose a reason for hiding this comment

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

likewise here.

opt = intrinEmbMask.baseType == TYP_DOUBLE ? INS_OPTS_D_TO_S : INS_OPTS_SCALABLE_S;
embOpt = intrinEmbMask.baseType == TYP_DOUBLE ? INS_OPTS_D_TO_S : INS_OPTS_SCALABLE_S;
break;
}
Copy link
Member

Choose a reason for hiding this comment

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

This also needs handling of ConvertToSingle

embOpt = intrinEmbMask.baseType == TYP_DOUBLE ? INS_OPTS_D_TO_S : INS_OPTS_SCALABLE_S;

and ConvertToDouble:

embOpt = intrinEmbMask.baseType == TYP_FLOAT ? INS_OPTS_S_TO_D : INS_OPTS_SCALABLE_D;

which means you can combine ConvertToSingle with Convert*32 and ConvertToDouble with Convert*64.

@ebepho ebepho changed the title Arm64/SVE: Implemented ConvertToint64, ConvertToUInt64, ConvertToDouble and ConvertToSingle Arm64/SVE: Implemented ConvertToint64 and ConvertToUInt64 Jun 28, 2024
@ebepho ebepho marked this pull request as ready for review June 28, 2024 00:54
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 around test. code changes looks good.

@@ -78,9 +78,9 @@
}
}";

const string SimpleVecOpTest_ValidationLogicForCndSelForNarrowing = @"for (var i = 0; i < Op1ElementCount; i++)
const string SimpleVecOpTest_ValidationLogicForCndSelNarrowing = @"for (var i = 0; i < Op1ElementCount; i++)
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

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 reverted this change -- shouldn't have been in this PR

("SveSimpleVecOpDifferentRetTypeTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_ConvertToInt32_int_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConvertToInt32", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Int32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetDouble()", ["ValidateIterResult"] = "Helpers.ConvertDoubleToInt32(firstOp[i]) != result[i]", ["GetIterResult"] = "Helpers.ConvertDoubleToInt32(leftOp[i])"}),
("SveSimpleVecOpDifferentRetTypeTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_ConvertToUInt32_uint_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConvertToUInt32", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetSingle()", ["ValidateIterResult"] = "Helpers.ConvertToUInt32(firstOp[i]) != result[i]", ["GetIterResult"] = "Helpers.ConvertToUInt32(leftOp[i])"}),
("SveSimpleVecOpDifferentRetTypeTest.template", new Dictionary<string, string> { ["TestName"] = "Sve_ConvertToUInt32_uint_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "ConvertToUInt32", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetDouble()", ["ValidateIterResult"] = "Helpers.ConvertDoubleToUInt32(firstOp[i]) != result[i]", ["GetIterResult"] = "Helpers.ConvertDoubleToUInt32(leftOp[i])"}),

Copy link
Member

Choose a reason for hiding this comment

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

can you delete the 2 empty lines?

@@ -177,7 +177,7 @@
("_BinaryOpTestTemplate.template", "SecureHashBinOpTest.template", new Dictionary<string, string> { ["TemplateName"] = "SecureHash", ["TemplateValidationLogic"] = SecureHashOpTest_ValidationLogic }),
("_TernaryOpTestTemplate.template", "SecureHashTernOpTest.template", new Dictionary<string, string> { ["TemplateName"] = "SecureHash", ["TemplateValidationLogic"] = SecureHashOpTest_ValidationLogic }),
("_SveUnaryOpTestTemplate.template", "SveSimpleVecOpTest.template", new Dictionary<string, string> { ["TemplateName"] = "Simple", ["TemplateValidationLogic"] = SimpleVecOpTest_ValidationLogic, ["TemplateValidationLogicForCndSel"] = SimpleVecOpTest_ValidationLogicForCndSel }),
("_SveUnaryOpDifferentRetTypeTestTemplate.template", "SveSimpleVecOpDifferentRetTypeTest.template", new Dictionary<string, string> { ["TemplateName"] = "Simple", ["TemplateValidationLogic"] = SimpleVecOpTest_ValidationLogicForNarrowing, ["TemplateValidationLogicForCndSel"] = SimpleVecOpTest_ValidationLogicForCndSelForNarrowing }),
("_SveUnaryOpDifferentRetTypeTestTemplate.template", "SveSimpleVecOpDifferentRetTypeTest.template", new Dictionary<string, string> { ["TemplateName"] = "Simple", ["TemplateValidationLogic"] = SimpleVecOpTest_ValidationLogicForNarrowing, ["TemplateValidationLogicForCndSel"] = SimpleVecOpTest_ValidationLogicForCndSelNarrowing }),
Copy link
Member

Choose a reason for hiding this comment

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

likewise here.

@@ -80,7 +80,7 @@

const string SimpleVecOpTest_ValidationLogicForCndSelForNarrowing = @"for (var i = 0; i < Op1ElementCount; i++)
{
{Op1BaseType} iterResult = (mask[i] != 0) ? {GetIterResult} : falseVal[i];
{RetBaseType} iterResult = (mask[i] != 0) ? {GetIterResult} : falseVal[i];
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed too?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{RetBaseType} iterResult = (mask[i] != 0) ? {GetIterResult} : falseVal[i];
{Op1BaseType} iterResult = (mask[i] != 0) ? {GetIterResult} : falseVal[i];

Copy link
Member

Choose a reason for hiding this comment

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

fixed it.

Copy link
Contributor Author

@ebepho ebepho Jun 28, 2024

Choose a reason for hiding this comment

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

I'm going to test this version one more time. Not sure why these changes even appeared, but I think they were supposed to be part of the ConvertToInt32 commit. These changes may be necessary for the tests to run properly.

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
Copy link
Member

/azp

Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@kunalspathak kunalspathak reopened this Jun 28, 2024
@kunalspathak kunalspathak merged commit 6f1d8c5 into dotnet:main Jun 29, 2024
159 of 167 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 29, 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.

2 participants