From 2346feb5e9c382c699936f6d8fafd1419ad5cdef Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Mon, 5 Aug 2024 16:57:57 +0100 Subject: [PATCH 01/18] ARM64-SVE: Add checks for MOVPRFX sequencing --- src/coreclr/jit/emit.cpp | 15 ++ src/coreclr/jit/emit.h | 4 + src/coreclr/jit/emitarm64.cpp | 202 ++++++++++++++++++ .../GenerateHWIntrinsicTests_Arm.cs | 4 +- 4 files changed, 223 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 6331adf3b420c4..609d5b7c57f2b5 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -7095,8 +7095,23 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, *instrCount = 0; jitstd::list::iterator nextMapping = emitComp->genRichIPmappings.begin(); #endif +#if defined(DEBUG) && defined(TARGET_ARM64) + instrDesc* prevId = nullptr; +#endif // DEBUG && TARGET_ARM64 + for (insGroup* ig = emitIGlist; ig != nullptr; ig = ig->igNext) { + +#if defined(DEBUG) && defined(TARGET_ARM64) + instrDesc* currId = emitFirstInstrDesc(ig->igData); + for (unsigned cnt = ig->igInsCnt; cnt > 0; cnt--) + { + emitInsPairSanityCheck(prevId, currId); + prevId = currId; + emitAdvanceInstrDesc(&currId, emitSizeOfInsDsc(currId)); + } +#endif // DEBUG && TARGET_ARM64 + assert(!(ig->igFlags & IGF_PLACEHOLDER)); // There better not be any placeholder groups left /* Is this the first cold block? */ diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index b227fb182d58b1..461c71b3a61e61 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -3128,6 +3128,10 @@ class emitter #ifndef TARGET_LOONGARCH64 void emitInsSanityCheck(instrDesc* id); #endif // TARGET_LOONGARCH64 + +#ifdef TARGET_ARM64 + void emitInsPairSanityCheck(instrDesc* prevId, instrDesc* id); +#endif #endif // DEBUG #ifdef TARGET_ARMARCH diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index d98270cd3081de..6488200490b826 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -957,6 +957,208 @@ void emitter::emitInsSanityCheck(instrDesc* id) break; } } + +/***************************************************************************** + * + * Sanity check two instruction are valid when placed next to each other + */ + +void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) +{ + if (firstId == nullptr || secondId == nullptr) + { + return; + } + + // Currently only concerned with instructions that follow movprfx + if (firstId->idIns() != INS_sve_movprfx) + { + return; + } + + // Quoted sections are taken fron the Arm Arm. + + // "It is required that the prefixed instruction at PC+4 must be an SVE destructive binary or ternary + // instruction encoding, or a unary operation with merging predication, but excluding other MOVPRFX instructions." + // "The prefixed instruction must not use the destination register in any other operand position, even if + // they have different names but refer to the same architectural register state." + switch (secondId->idInsFmt()) + { + case IF_SVE_AM_2A: // ., /M, ., # + case IF_SVE_BN_1A: // .D{, {, MUL #}} + case IF_SVE_BP_1A: // .D{, {, MUL #}} + case IF_SVE_BS_1A: // ., ., # + case IF_SVE_BU_2A: // ., /M, # + case IF_SVE_BV_2A_A: // ., /M, #{, } + case IF_SVE_BV_2A_J: // ., /M, #{, } + case IF_SVE_BV_2B: // ., /M, #0.0 + case IF_SVE_CD_2A: // ., + case IF_SVE_CQ_3A: // ., /M, + case IF_SVE_DN_2A: // ., . + case IF_SVE_DP_2A: // ., . + case IF_SVE_EC_1A: // ., ., #{, } + case IF_SVE_ED_1A: // ., ., # + case IF_SVE_EE_1A: // ., ., # + case IF_SVE_HM_2A: // ., /M, ., + break; + + case IF_SVE_CC_2A: // ., + case IF_SVE_FU_2A: // ., ., # + assert(secondId->idReg1() != secondId->idReg2()); + break; + + case IF_SVE_AP_3A: // ., /M, . + case IF_SVE_AQ_3A: // ., /M, . + case IF_SVE_AW_2A: // ., ., ., # + case IF_SVE_CP_3A: // ., /M, + case IF_SVE_CT_3A: // .Q, /M, .Q + case IF_SVE_CU_3A: // ., /M, . + case IF_SVE_ES_3A: // ., /M, . + case IF_SVE_EQ_3A: // ., /M, . + case IF_SVE_FV_2A: // ., ., ., + case IF_SVE_HN_2A: // ., ., ., # + case IF_SVE_HO_3A: // .H, /M, .S + case IF_SVE_HO_3B: // .D, /M, .S + case IF_SVE_HO_3C: // .S, /M, .D + case IF_SVE_HP_3A: // ., /M, . + case IF_SVE_HP_3B: // ., /M, . + case IF_SVE_HQ_3A: // ., /M, . + case IF_SVE_HR_3A: // ., /M, . + case IF_SVE_HS_3A: // ., /M, . + assert(secondId->idReg1() != secondId->idReg3()); + break; + + case IF_SVE_BY_2A: // .B, .B, .B, # + case IF_SVE_EF_3A: // .S, .H, .H + case IF_SVE_EG_3A: // .S, .H, .H[] + case IF_SVE_EH_3A: // ., ., . + case IF_SVE_EI_3A: // .S, .B, .B + case IF_SVE_EJ_3A: // ., ., ., + case IF_SVE_EK_3A: // ., ., ., + case IF_SVE_EL_3A: // ., ., . + case IF_SVE_EM_3A: // ., ., . + case IF_SVE_EW_3A: // .D, .D, .D + case IF_SVE_EW_3B: // .D, .D, .D + case IF_SVE_EY_3A: // .S, .B, .B[] + case IF_SVE_EY_3B: // .D, .H, .H[] + case IF_SVE_EZ_3A: // .S, .B, .B[] + case IF_SVE_FA_3A: // .S, .B, .B[], + case IF_SVE_FA_3B: // .D, .H, .H[], + case IF_SVE_FB_3A: // .H, .H, .H[], + case IF_SVE_FB_3B: // .S, .S, .S[], + case IF_SVE_FC_3A: // .H, .H, .H[], + case IF_SVE_FC_3B: // .S, .S, .S[], + case IF_SVE_FF_3A: // .H, .H, .H[] + case IF_SVE_FF_3B: // .S, .S, .S[] + case IF_SVE_FF_3C: // .D, .D, .D[] + case IF_SVE_FG_3A: // .S, .H, .H[] + case IF_SVE_FG_3B: // .D, .S, .S[] + case IF_SVE_FJ_3A: // .S, .H, .H[] + case IF_SVE_FJ_3B: // .D, .S, .S[] + case IF_SVE_FK_3A: // .H, .H, .H[] + case IF_SVE_FK_3B: // .S, .S, .S[] + case IF_SVE_FK_3C: // .D, .D, .D[] + case IF_SVE_FO_3A: // .S, .B, .B + case IF_SVE_FW_3A: // ., ., . + case IF_SVE_FY_3A: // ., ., . + case IF_SVE_GM_3A: // .H, .B, .B[] + case IF_SVE_GN_3A: // .H, .B, .B + case IF_SVE_GO_3A: // .S, .B, .B + case IF_SVE_GU_3A: // .S, .S, .S[] + case IF_SVE_GU_3B: // .D, .D, .D[] + case IF_SVE_GU_3C: // .H, .H, .H[] + case IF_SVE_GV_3A: // .S, .S, .S[], + case IF_SVE_GW_3B: // .H, .H, .H + case IF_SVE_GY_3A: // .H, .B, .B[] + case IF_SVE_GY_3B: // .S, .H, .H[] + case IF_SVE_GY_3B_D: // .S, .B, .B[] + case IF_SVE_GZ_3A: // .S, .H, .H[] + case IF_SVE_HA_3A: // .S, .H, .H + case IF_SVE_HA_3A_E: // .H, .B, .B + case IF_SVE_HA_3A_F: // .S, .B, .B + case IF_SVE_HB_3A: // .S, .H, .H + case IF_SVE_HC_3A: // .S, .B, .B[] + case IF_SVE_HD_3A: // .S, .H, .H + case IF_SVE_HD_3A_A: // .D, .D, .D + assert(secondId->idReg1() != secondId->idReg2()); + assert(secondId->idReg1() != secondId->idReg3()); + break; + + case IF_SVE_AB_3B: // .D, /M, .D, .D + case IF_SVE_AA_3A: // ., /M, ., . + case IF_SVE_AC_3A: // ., /M, ., . + case IF_SVE_AO_3A: // ., /M, ., .D + case IF_SVE_CM_3A: // ., , ., . + case IF_SVE_HL_3A: // ., /M, ., . + case IF_SVE_HL_3B: // .H, /M, .H, .H + assert(secondId->idReg1() != secondId->idReg4()); + break; + + case IF_SVE_AV_3A: // .D, .D, .D, .D + assert(secondId->idReg1() != secondId->idReg2()); + assert(secondId->idReg1() != secondId->idReg3()); + assert(secondId->idReg1() != secondId->idReg4()); + break; + + case IF_SVE_AR_4A: // ., /M, ., . + case IF_SVE_AS_4A: // ., /M, ., . + case IF_SVE_GP_3A: // ., /M, ., ., + case IF_SVE_GR_3A: // ., /M, ., . + case IF_SVE_GT_4A: // ., /M, ., ., + case IF_SVE_HU_4A: // ., /M, ., . + case IF_SVE_HU_4B: // .H, /M, .H, .H + case IF_SVE_HV_4A: // ., /M, ., . + assert(secondId->idReg1() != secondId->idReg3()); + assert(secondId->idReg1() != secondId->idReg4()); + break; + + case IF_SVE_AT_3A: // ., ., . + // Only a subset of this group is valid + switch (secondId->idIns()) + { + case INS_sve_sclamp: + case INS_sve_uclamp: + case INS_sve_eorbt: + case INS_sve_eortb: + case INS_sve_fclamp: + break; + default: + assert(false); + } + assert(secondId->idReg1() != secondId->idReg2()); + assert(secondId->idReg1() != secondId->idReg3()); + break; + + default: + assert(false); + break; + } + + if (firstId->idInsFmt() == IF_SVE_AH_3A) + { + // 3 operand version + + // "The prefixed instruction must specify the same predicate register" + assert(isPredicateRegister(firstId->idReg2())); + assert(isPredicateRegister(secondId->idReg2())); + assert(firstId->idReg2() == secondId->idReg2()); + + // "and have the same maximum element size (ignoring a fixed 64-bit "wide vector" operand)," + assert(firstId->idInsOpt() == secondId->idInsOpt()); + + // "and the same destination vector as the MOVPRFX instruction." + assert(firstId->idReg1() == secondId->idReg1()); + } + else + { + // 2 operand version + assert(firstId->idInsFmt() == IF_SVE_BI_2A); + + // "The prefixed instruction must specify the same destination vector as the MOVPRFX instruction." + assert(firstId->idReg1() == secondId->idReg1()); + } +} + #endif // DEBUG bool emitter::emitInsMayWriteToGCReg(instrDesc* id) diff --git a/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs b/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs index f6ed31fb87e62f..fb1fbb24d28165 100644 --- a/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs +++ b/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs @@ -3131,8 +3131,8 @@ ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_AddSaturate_uint", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSaturate", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt32", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "UInt32", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()", ["NextValueOp2"] = "TestLibrary.Generator.GetUInt32()", ["ValidateIterResult"] = "Helpers.AddSaturate(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.AddSaturate(left[i], right[i])"}), ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_AddSaturate_ulong", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSaturate", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt64", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt64", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "UInt64", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt64()", ["NextValueOp2"] = "TestLibrary.Generator.GetUInt64()", ["ValidateIterResult"] = "Helpers.AddSaturate(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.AddSaturate(left[i], right[i])"}), - ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), - ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Double", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskDouble()", ["NextValueOp2"] = "TestLibrary.Generator.GetDouble()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), + // ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), + // ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Double", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskDouble()", ["NextValueOp2"] = "TestLibrary.Generator.GetDouble()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_And_sbyte", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "And", ["RetVectorType"] = "Vector", ["RetBaseType"] = "SByte", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "SByte", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "SByte", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "(sbyte)TestLibrary.Generator.GetSByte()", ["NextValueOp2"] = "TestLibrary.Generator.GetSByte()", ["ValidateIterResult"] = "Helpers.And(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.And(left[i], right[i])"}), ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_And_short", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "And", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Int16", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Int16", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Int16", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "(short)TestLibrary.Generator.GetInt16()", ["NextValueOp2"] = "TestLibrary.Generator.GetInt16()", ["ValidateIterResult"] = "Helpers.And(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.And(left[i], right[i])"}), From aa2473438e179874011c086ccccc4fca173545a8 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Fri, 9 Aug 2024 12:40:43 +0100 Subject: [PATCH 02/18] Fix formatting --- src/coreclr/jit/emitarm64.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 6488200490b826..592c1a4beeb410 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -1028,8 +1028,8 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) assert(secondId->idReg1() != secondId->idReg3()); break; - case IF_SVE_BY_2A: // .B, .B, .B, # - case IF_SVE_EF_3A: // .S, .H, .H + case IF_SVE_BY_2A: // .B, .B, .B, # + case IF_SVE_EF_3A: // .S, .H, .H case IF_SVE_EG_3A: // .S, .H, .H[] case IF_SVE_EH_3A: // ., ., . case IF_SVE_EI_3A: // .S, .B, .B From 4c7420ab96ec9f3d88ada098899084d083922286 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Fri, 9 Aug 2024 17:14:09 +0100 Subject: [PATCH 03/18] Move emitInsPairSanityCheck --- src/coreclr/jit/emitarm64.cpp | 202 ------------------------------ src/coreclr/jit/emitarm64sve.cpp | 203 +++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 202 deletions(-) diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 592c1a4beeb410..d98270cd3081de 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -957,208 +957,6 @@ void emitter::emitInsSanityCheck(instrDesc* id) break; } } - -/***************************************************************************** - * - * Sanity check two instruction are valid when placed next to each other - */ - -void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) -{ - if (firstId == nullptr || secondId == nullptr) - { - return; - } - - // Currently only concerned with instructions that follow movprfx - if (firstId->idIns() != INS_sve_movprfx) - { - return; - } - - // Quoted sections are taken fron the Arm Arm. - - // "It is required that the prefixed instruction at PC+4 must be an SVE destructive binary or ternary - // instruction encoding, or a unary operation with merging predication, but excluding other MOVPRFX instructions." - // "The prefixed instruction must not use the destination register in any other operand position, even if - // they have different names but refer to the same architectural register state." - switch (secondId->idInsFmt()) - { - case IF_SVE_AM_2A: // ., /M, ., # - case IF_SVE_BN_1A: // .D{, {, MUL #}} - case IF_SVE_BP_1A: // .D{, {, MUL #}} - case IF_SVE_BS_1A: // ., ., # - case IF_SVE_BU_2A: // ., /M, # - case IF_SVE_BV_2A_A: // ., /M, #{, } - case IF_SVE_BV_2A_J: // ., /M, #{, } - case IF_SVE_BV_2B: // ., /M, #0.0 - case IF_SVE_CD_2A: // ., - case IF_SVE_CQ_3A: // ., /M, - case IF_SVE_DN_2A: // ., . - case IF_SVE_DP_2A: // ., . - case IF_SVE_EC_1A: // ., ., #{, } - case IF_SVE_ED_1A: // ., ., # - case IF_SVE_EE_1A: // ., ., # - case IF_SVE_HM_2A: // ., /M, ., - break; - - case IF_SVE_CC_2A: // ., - case IF_SVE_FU_2A: // ., ., # - assert(secondId->idReg1() != secondId->idReg2()); - break; - - case IF_SVE_AP_3A: // ., /M, . - case IF_SVE_AQ_3A: // ., /M, . - case IF_SVE_AW_2A: // ., ., ., # - case IF_SVE_CP_3A: // ., /M, - case IF_SVE_CT_3A: // .Q, /M, .Q - case IF_SVE_CU_3A: // ., /M, . - case IF_SVE_ES_3A: // ., /M, . - case IF_SVE_EQ_3A: // ., /M, . - case IF_SVE_FV_2A: // ., ., ., - case IF_SVE_HN_2A: // ., ., ., # - case IF_SVE_HO_3A: // .H, /M, .S - case IF_SVE_HO_3B: // .D, /M, .S - case IF_SVE_HO_3C: // .S, /M, .D - case IF_SVE_HP_3A: // ., /M, . - case IF_SVE_HP_3B: // ., /M, . - case IF_SVE_HQ_3A: // ., /M, . - case IF_SVE_HR_3A: // ., /M, . - case IF_SVE_HS_3A: // ., /M, . - assert(secondId->idReg1() != secondId->idReg3()); - break; - - case IF_SVE_BY_2A: // .B, .B, .B, # - case IF_SVE_EF_3A: // .S, .H, .H - case IF_SVE_EG_3A: // .S, .H, .H[] - case IF_SVE_EH_3A: // ., ., . - case IF_SVE_EI_3A: // .S, .B, .B - case IF_SVE_EJ_3A: // ., ., ., - case IF_SVE_EK_3A: // ., ., ., - case IF_SVE_EL_3A: // ., ., . - case IF_SVE_EM_3A: // ., ., . - case IF_SVE_EW_3A: // .D, .D, .D - case IF_SVE_EW_3B: // .D, .D, .D - case IF_SVE_EY_3A: // .S, .B, .B[] - case IF_SVE_EY_3B: // .D, .H, .H[] - case IF_SVE_EZ_3A: // .S, .B, .B[] - case IF_SVE_FA_3A: // .S, .B, .B[], - case IF_SVE_FA_3B: // .D, .H, .H[], - case IF_SVE_FB_3A: // .H, .H, .H[], - case IF_SVE_FB_3B: // .S, .S, .S[], - case IF_SVE_FC_3A: // .H, .H, .H[], - case IF_SVE_FC_3B: // .S, .S, .S[], - case IF_SVE_FF_3A: // .H, .H, .H[] - case IF_SVE_FF_3B: // .S, .S, .S[] - case IF_SVE_FF_3C: // .D, .D, .D[] - case IF_SVE_FG_3A: // .S, .H, .H[] - case IF_SVE_FG_3B: // .D, .S, .S[] - case IF_SVE_FJ_3A: // .S, .H, .H[] - case IF_SVE_FJ_3B: // .D, .S, .S[] - case IF_SVE_FK_3A: // .H, .H, .H[] - case IF_SVE_FK_3B: // .S, .S, .S[] - case IF_SVE_FK_3C: // .D, .D, .D[] - case IF_SVE_FO_3A: // .S, .B, .B - case IF_SVE_FW_3A: // ., ., . - case IF_SVE_FY_3A: // ., ., . - case IF_SVE_GM_3A: // .H, .B, .B[] - case IF_SVE_GN_3A: // .H, .B, .B - case IF_SVE_GO_3A: // .S, .B, .B - case IF_SVE_GU_3A: // .S, .S, .S[] - case IF_SVE_GU_3B: // .D, .D, .D[] - case IF_SVE_GU_3C: // .H, .H, .H[] - case IF_SVE_GV_3A: // .S, .S, .S[], - case IF_SVE_GW_3B: // .H, .H, .H - case IF_SVE_GY_3A: // .H, .B, .B[] - case IF_SVE_GY_3B: // .S, .H, .H[] - case IF_SVE_GY_3B_D: // .S, .B, .B[] - case IF_SVE_GZ_3A: // .S, .H, .H[] - case IF_SVE_HA_3A: // .S, .H, .H - case IF_SVE_HA_3A_E: // .H, .B, .B - case IF_SVE_HA_3A_F: // .S, .B, .B - case IF_SVE_HB_3A: // .S, .H, .H - case IF_SVE_HC_3A: // .S, .B, .B[] - case IF_SVE_HD_3A: // .S, .H, .H - case IF_SVE_HD_3A_A: // .D, .D, .D - assert(secondId->idReg1() != secondId->idReg2()); - assert(secondId->idReg1() != secondId->idReg3()); - break; - - case IF_SVE_AB_3B: // .D, /M, .D, .D - case IF_SVE_AA_3A: // ., /M, ., . - case IF_SVE_AC_3A: // ., /M, ., . - case IF_SVE_AO_3A: // ., /M, ., .D - case IF_SVE_CM_3A: // ., , ., . - case IF_SVE_HL_3A: // ., /M, ., . - case IF_SVE_HL_3B: // .H, /M, .H, .H - assert(secondId->idReg1() != secondId->idReg4()); - break; - - case IF_SVE_AV_3A: // .D, .D, .D, .D - assert(secondId->idReg1() != secondId->idReg2()); - assert(secondId->idReg1() != secondId->idReg3()); - assert(secondId->idReg1() != secondId->idReg4()); - break; - - case IF_SVE_AR_4A: // ., /M, ., . - case IF_SVE_AS_4A: // ., /M, ., . - case IF_SVE_GP_3A: // ., /M, ., ., - case IF_SVE_GR_3A: // ., /M, ., . - case IF_SVE_GT_4A: // ., /M, ., ., - case IF_SVE_HU_4A: // ., /M, ., . - case IF_SVE_HU_4B: // .H, /M, .H, .H - case IF_SVE_HV_4A: // ., /M, ., . - assert(secondId->idReg1() != secondId->idReg3()); - assert(secondId->idReg1() != secondId->idReg4()); - break; - - case IF_SVE_AT_3A: // ., ., . - // Only a subset of this group is valid - switch (secondId->idIns()) - { - case INS_sve_sclamp: - case INS_sve_uclamp: - case INS_sve_eorbt: - case INS_sve_eortb: - case INS_sve_fclamp: - break; - default: - assert(false); - } - assert(secondId->idReg1() != secondId->idReg2()); - assert(secondId->idReg1() != secondId->idReg3()); - break; - - default: - assert(false); - break; - } - - if (firstId->idInsFmt() == IF_SVE_AH_3A) - { - // 3 operand version - - // "The prefixed instruction must specify the same predicate register" - assert(isPredicateRegister(firstId->idReg2())); - assert(isPredicateRegister(secondId->idReg2())); - assert(firstId->idReg2() == secondId->idReg2()); - - // "and have the same maximum element size (ignoring a fixed 64-bit "wide vector" operand)," - assert(firstId->idInsOpt() == secondId->idInsOpt()); - - // "and the same destination vector as the MOVPRFX instruction." - assert(firstId->idReg1() == secondId->idReg1()); - } - else - { - // 2 operand version - assert(firstId->idInsFmt() == IF_SVE_BI_2A); - - // "The prefixed instruction must specify the same destination vector as the MOVPRFX instruction." - assert(firstId->idReg1() == secondId->idReg1()); - } -} - #endif // DEBUG bool emitter::emitInsMayWriteToGCReg(instrDesc* id) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 714c5716e1cd17..c33f5e7362c20b 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18421,4 +18421,207 @@ void emitter::getInsSveExecutionCharacteristics(instrDesc* id, insExecutionChara } #endif // defined(DEBUG) || defined(LATE_DISASM) +#ifdef DEBUG +/***************************************************************************** + * + * Sanity check two instruction are valid when placed next to each other + */ + +void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) +{ + if (firstId == nullptr || secondId == nullptr) + { + return; + } + + // Currently only concerned with instructions that follow movprfx + if (firstId->idIns() != INS_sve_movprfx) + { + return; + } + + // Quoted sections are taken fron the Arm manual. + + // "It is required that the prefixed instruction at PC+4 must be an SVE destructive binary or ternary + // instruction encoding, or a unary operation with merging predication, but excluding other MOVPRFX instructions." + // "The prefixed instruction must not use the destination register in any other operand position, even if + // they have different names but refer to the same architectural register state." + switch (secondId->idInsFmt()) + { + case IF_SVE_AM_2A: // ., /M, ., # + case IF_SVE_BN_1A: // .D{, {, MUL #}} + case IF_SVE_BP_1A: // .D{, {, MUL #}} + case IF_SVE_BS_1A: // ., ., # + case IF_SVE_BU_2A: // ., /M, # + case IF_SVE_BV_2A_A: // ., /M, #{, } + case IF_SVE_BV_2A_J: // ., /M, #{, } + case IF_SVE_BV_2B: // ., /M, #0.0 + case IF_SVE_CD_2A: // ., + case IF_SVE_CQ_3A: // ., /M, + case IF_SVE_DN_2A: // ., . + case IF_SVE_DP_2A: // ., . + case IF_SVE_EC_1A: // ., ., #{, } + case IF_SVE_ED_1A: // ., ., # + case IF_SVE_EE_1A: // ., ., # + case IF_SVE_HM_2A: // ., /M, ., + break; + + case IF_SVE_CC_2A: // ., + case IF_SVE_FU_2A: // ., ., # + assert(secondId->idReg1() != secondId->idReg2()); + break; + + case IF_SVE_AP_3A: // ., /M, . + case IF_SVE_AQ_3A: // ., /M, . + case IF_SVE_AW_2A: // ., ., ., # + case IF_SVE_CP_3A: // ., /M, + case IF_SVE_CT_3A: // .Q, /M, .Q + case IF_SVE_CU_3A: // ., /M, . + case IF_SVE_ES_3A: // ., /M, . + case IF_SVE_EQ_3A: // ., /M, . + case IF_SVE_FV_2A: // ., ., ., + case IF_SVE_HN_2A: // ., ., ., # + case IF_SVE_HO_3A: // .H, /M, .S + case IF_SVE_HO_3B: // .D, /M, .S + case IF_SVE_HO_3C: // .S, /M, .D + case IF_SVE_HP_3A: // ., /M, . + case IF_SVE_HP_3B: // ., /M, . + case IF_SVE_HQ_3A: // ., /M, . + case IF_SVE_HR_3A: // ., /M, . + case IF_SVE_HS_3A: // ., /M, . + assert(secondId->idReg1() != secondId->idReg3()); + break; + + case IF_SVE_BY_2A: // .B, .B, .B, # + case IF_SVE_EF_3A: // .S, .H, .H + case IF_SVE_EG_3A: // .S, .H, .H[] + case IF_SVE_EH_3A: // ., ., . + case IF_SVE_EI_3A: // .S, .B, .B + case IF_SVE_EJ_3A: // ., ., ., + case IF_SVE_EK_3A: // ., ., ., + case IF_SVE_EL_3A: // ., ., . + case IF_SVE_EM_3A: // ., ., . + case IF_SVE_EW_3A: // .D, .D, .D + case IF_SVE_EW_3B: // .D, .D, .D + case IF_SVE_EY_3A: // .S, .B, .B[] + case IF_SVE_EY_3B: // .D, .H, .H[] + case IF_SVE_EZ_3A: // .S, .B, .B[] + case IF_SVE_FA_3A: // .S, .B, .B[], + case IF_SVE_FA_3B: // .D, .H, .H[], + case IF_SVE_FB_3A: // .H, .H, .H[], + case IF_SVE_FB_3B: // .S, .S, .S[], + case IF_SVE_FC_3A: // .H, .H, .H[], + case IF_SVE_FC_3B: // .S, .S, .S[], + case IF_SVE_FF_3A: // .H, .H, .H[] + case IF_SVE_FF_3B: // .S, .S, .S[] + case IF_SVE_FF_3C: // .D, .D, .D[] + case IF_SVE_FG_3A: // .S, .H, .H[] + case IF_SVE_FG_3B: // .D, .S, .S[] + case IF_SVE_FJ_3A: // .S, .H, .H[] + case IF_SVE_FJ_3B: // .D, .S, .S[] + case IF_SVE_FK_3A: // .H, .H, .H[] + case IF_SVE_FK_3B: // .S, .S, .S[] + case IF_SVE_FK_3C: // .D, .D, .D[] + case IF_SVE_FO_3A: // .S, .B, .B + case IF_SVE_FW_3A: // ., ., . + case IF_SVE_FY_3A: // ., ., . + case IF_SVE_GM_3A: // .H, .B, .B[] + case IF_SVE_GN_3A: // .H, .B, .B + case IF_SVE_GO_3A: // .S, .B, .B + case IF_SVE_GU_3A: // .S, .S, .S[] + case IF_SVE_GU_3B: // .D, .D, .D[] + case IF_SVE_GU_3C: // .H, .H, .H[] + case IF_SVE_GV_3A: // .S, .S, .S[], + case IF_SVE_GW_3B: // .H, .H, .H + case IF_SVE_GY_3A: // .H, .B, .B[] + case IF_SVE_GY_3B: // .S, .H, .H[] + case IF_SVE_GY_3B_D: // .S, .B, .B[] + case IF_SVE_GZ_3A: // .S, .H, .H[] + case IF_SVE_HA_3A: // .S, .H, .H + case IF_SVE_HA_3A_E: // .H, .B, .B + case IF_SVE_HA_3A_F: // .S, .B, .B + case IF_SVE_HB_3A: // .S, .H, .H + case IF_SVE_HC_3A: // .S, .B, .B[] + case IF_SVE_HD_3A: // .S, .H, .H + case IF_SVE_HD_3A_A: // .D, .D, .D + assert(secondId->idReg1() != secondId->idReg2()); + assert(secondId->idReg1() != secondId->idReg3()); + break; + + case IF_SVE_AB_3B: // .D, /M, .D, .D + case IF_SVE_AA_3A: // ., /M, ., . + case IF_SVE_AC_3A: // ., /M, ., . + case IF_SVE_AO_3A: // ., /M, ., .D + case IF_SVE_CM_3A: // ., , ., . + case IF_SVE_HL_3A: // ., /M, ., . + case IF_SVE_HL_3B: // .H, /M, .H, .H + assert(secondId->idReg1() != secondId->idReg4()); + break; + + case IF_SVE_AV_3A: // .D, .D, .D, .D + assert(secondId->idReg1() != secondId->idReg2()); + assert(secondId->idReg1() != secondId->idReg3()); + assert(secondId->idReg1() != secondId->idReg4()); + break; + + case IF_SVE_AR_4A: // ., /M, ., . + case IF_SVE_AS_4A: // ., /M, ., . + case IF_SVE_GP_3A: // ., /M, ., ., + case IF_SVE_GR_3A: // ., /M, ., . + case IF_SVE_GT_4A: // ., /M, ., ., + case IF_SVE_HU_4A: // ., /M, ., . + case IF_SVE_HU_4B: // .H, /M, .H, .H + case IF_SVE_HV_4A: // ., /M, ., . + assert(secondId->idReg1() != secondId->idReg3()); + assert(secondId->idReg1() != secondId->idReg4()); + break; + + case IF_SVE_AT_3A: // ., ., . + // Only a subset of this group is valid + switch (secondId->idIns()) + { + case INS_sve_sclamp: + case INS_sve_uclamp: + case INS_sve_eorbt: + case INS_sve_eortb: + case INS_sve_fclamp: + break; + default: + assert(false); + } + assert(secondId->idReg1() != secondId->idReg2()); + assert(secondId->idReg1() != secondId->idReg3()); + break; + + default: + assert(false); + break; + } + + if (firstId->idInsFmt() == IF_SVE_AH_3A) + { + // 3 operand version + + // "The prefixed instruction must specify the same predicate register" + assert(isPredicateRegister(firstId->idReg2())); + assert(isPredicateRegister(secondId->idReg2())); + assert(firstId->idReg2() == secondId->idReg2()); + + // "and have the same maximum element size (ignoring a fixed 64-bit "wide vector" operand)," + assert(firstId->idInsOpt() == secondId->idInsOpt()); + + // "and the same destination vector as the MOVPRFX instruction." + assert(firstId->idReg1() == secondId->idReg1()); + } + else + { + // 2 operand version + assert(firstId->idInsFmt() == IF_SVE_BI_2A); + + // "The prefixed instruction must specify the same destination vector as the MOVPRFX instruction." + assert(firstId->idReg1() == secondId->idReg1()); + } +} +#endif // DEBUG + #endif // TARGET_ARM64 From d33578fd0fa289edea1013541ec8b7aeaef94914 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Mon, 12 Aug 2024 11:38:09 +0100 Subject: [PATCH 04/18] Improve asserts --- src/coreclr/jit/emitarm64sve.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index c33f5e7362c20b..6b9d348b0680ec 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18587,14 +18587,14 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case INS_sve_fclamp: break; default: - assert(false); + assert(!"Got unexpected instruction format within group after MOVPRFX"); } assert(secondId->idReg1() != secondId->idReg2()); assert(secondId->idReg1() != secondId->idReg3()); break; default: - assert(false); + assert(!"Got unexpected instruction format after MOVPRFX"); break; } From bf3e08ef34562062ed933ef344272241246046c7 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Mon, 12 Aug 2024 12:14:30 +0100 Subject: [PATCH 05/18] check final instruction --- src/coreclr/jit/emit.cpp | 5 +++++ src/coreclr/jit/emitarm64sve.cpp | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 609d5b7c57f2b5..91f3b40568e91f 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -7110,6 +7110,11 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, prevId = currId; emitAdvanceInstrDesc(&currId, emitSizeOfInsDsc(currId)); } + // Additional check for the final instruction + if (ig->igNext == nullptr) + { + emitInsPairSanityCheck(prevId, nullptr); + } #endif // DEBUG && TARGET_ARM64 assert(!(ig->igFlags & IGF_PLACEHOLDER)); // There better not be any placeholder groups left diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 6b9d348b0680ec..3d5b988fa32be0 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18429,7 +18429,7 @@ void emitter::getInsSveExecutionCharacteristics(instrDesc* id, insExecutionChara void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) { - if (firstId == nullptr || secondId == nullptr) + if (firstId == nullptr) { return; } @@ -18440,6 +18440,12 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) return; } + // The movprfx can't be the last instruction + if (secondId == nullptr) + { + assert(!"Last instruction generated is a MOVPRFX"); + } + // Quoted sections are taken fron the Arm manual. // "It is required that the prefixed instruction at PC+4 must be an SVE destructive binary or ternary From 2e46d8d793d40a9b0c91ece993ed63bf6a19672f Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Mon, 12 Aug 2024 14:00:08 +0100 Subject: [PATCH 06/18] Reorder switch table and add movprefxIsPredicated check --- src/coreclr/jit/emitarm64sve.cpp | 68 +++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 3d5b988fa32be0..8a1e7c24d8f55f 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18446,6 +18446,17 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) assert(!"Last instruction generated is a MOVPRFX"); } + bool movprefxIsPredicated = false; + if (firstId->idInsFmt() == IF_SVE_AH_3A) + { + movprefxIsPredicated = true; + } + else + { + // 2 operand version + assert(firstId->idInsFmt() == IF_SVE_BI_2A); + } + // Quoted sections are taken fron the Arm manual. // "It is required that the prefixed instruction at PC+4 must be an SVE destructive binary or ternary @@ -18454,51 +18465,64 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) // they have different names but refer to the same architectural register state." switch (secondId->idInsFmt()) { - case IF_SVE_AM_2A: // ., /M, ., # - case IF_SVE_BN_1A: // .D{, {, MUL #}} - case IF_SVE_BP_1A: // .D{, {, MUL #}} - case IF_SVE_BS_1A: // ., ., # + case IF_SVE_BN_1A: // .D{, {, MUL #}} + case IF_SVE_BP_1A: // .D{, {, MUL #}} + case IF_SVE_CC_2A: // ., + case IF_SVE_CD_2A: // ., + case IF_SVE_DN_2A: // ., . + case IF_SVE_DP_2A: // ., . + // Tied registers + case IF_SVE_BS_1A: // ., ., # + case IF_SVE_EC_1A: // ., ., #{, } + case IF_SVE_ED_1A: // ., ., # + case IF_SVE_EE_1A: // ., ., # + assert(!movprefxIsPredicated); + break; + case IF_SVE_BU_2A: // ., /M, # case IF_SVE_BV_2A_A: // ., /M, #{, } case IF_SVE_BV_2A_J: // ., /M, #{, } case IF_SVE_BV_2B: // ., /M, #0.0 - case IF_SVE_CD_2A: // ., case IF_SVE_CQ_3A: // ., /M, - case IF_SVE_DN_2A: // ., . - case IF_SVE_DP_2A: // ., . - case IF_SVE_EC_1A: // ., ., #{, } - case IF_SVE_ED_1A: // ., ., # - case IF_SVE_EE_1A: // ., ., # + // Tied registers + case IF_SVE_AM_2A: // ., /M, ., # case IF_SVE_HM_2A: // ., /M, ., + assert(movprefxIsPredicated); break; - case IF_SVE_CC_2A: // ., case IF_SVE_FU_2A: // ., ., # + assert(!movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg2()); break; case IF_SVE_AP_3A: // ., /M, . case IF_SVE_AQ_3A: // ., /M, . - case IF_SVE_AW_2A: // ., ., ., # case IF_SVE_CP_3A: // ., /M, case IF_SVE_CT_3A: // .Q, /M, .Q case IF_SVE_CU_3A: // ., /M, . case IF_SVE_ES_3A: // ., /M, . case IF_SVE_EQ_3A: // ., /M, . - case IF_SVE_FV_2A: // ., ., ., - case IF_SVE_HN_2A: // ., ., ., # case IF_SVE_HO_3A: // .H, /M, .S case IF_SVE_HO_3B: // .D, /M, .S case IF_SVE_HO_3C: // .S, /M, .D case IF_SVE_HP_3A: // ., /M, . - case IF_SVE_HP_3B: // ., /M, . case IF_SVE_HQ_3A: // ., /M, . case IF_SVE_HR_3A: // ., /M, . case IF_SVE_HS_3A: // ., /M, . + case IF_SVE_HP_3B: // ., /M, . + assert(movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg3()); break; + // Tied registers + case IF_SVE_AW_2A: // ., ., ., # case IF_SVE_BY_2A: // .B, .B, .B, # + case IF_SVE_FV_2A: // ., ., ., + case IF_SVE_HN_2A: // ., ., ., # + assert(!movprefxIsPredicated); + assert(secondId->idReg1() != secondId->idReg3()); + break; + case IF_SVE_EF_3A: // .S, .H, .H case IF_SVE_EG_3A: // .S, .H, .H[] case IF_SVE_EH_3A: // ., ., . @@ -18550,34 +18574,39 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_HC_3A: // .S, .B, .B[] case IF_SVE_HD_3A: // .S, .H, .H case IF_SVE_HD_3A_A: // .D, .D, .D + assert(!movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg2()); assert(secondId->idReg1() != secondId->idReg3()); break; - case IF_SVE_AB_3B: // .D, /M, .D, .D + // Tied registers case IF_SVE_AA_3A: // ., /M, ., . + case IF_SVE_AB_3B: // .D, /M, .D, .D case IF_SVE_AC_3A: // ., /M, ., . case IF_SVE_AO_3A: // ., /M, ., .D case IF_SVE_CM_3A: // ., , ., . + case IF_SVE_GP_3A: // ., /M, ., ., + case IF_SVE_GR_3A: // ., /M, ., . case IF_SVE_HL_3A: // ., /M, ., . case IF_SVE_HL_3B: // .H, /M, .H, .H + assert(movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg4()); break; + // Tied registers case IF_SVE_AV_3A: // .D, .D, .D, .D - assert(secondId->idReg1() != secondId->idReg2()); + assert(!movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg3()); assert(secondId->idReg1() != secondId->idReg4()); break; case IF_SVE_AR_4A: // ., /M, ., . case IF_SVE_AS_4A: // ., /M, ., . - case IF_SVE_GP_3A: // ., /M, ., ., - case IF_SVE_GR_3A: // ., /M, ., . case IF_SVE_GT_4A: // ., /M, ., ., case IF_SVE_HU_4A: // ., /M, ., . case IF_SVE_HU_4B: // .H, /M, .H, .H case IF_SVE_HV_4A: // ., /M, ., . + assert(movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg3()); assert(secondId->idReg1() != secondId->idReg4()); break; @@ -18595,6 +18624,7 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) default: assert(!"Got unexpected instruction format within group after MOVPRFX"); } + assert(!movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg2()); assert(secondId->idReg1() != secondId->idReg3()); break; From 5a517f6a4d2cbbdce91bb65aaca7c97f5be25bc5 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Mon, 12 Aug 2024 18:02:19 +0100 Subject: [PATCH 07/18] fix formatting --- src/coreclr/jit/emitarm64sve.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 8a1e7c24d8f55f..4b9ebf0191e11c 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18441,7 +18441,7 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) } // The movprfx can't be the last instruction - if (secondId == nullptr) + if (secondId == nullptr) { assert(!"Last instruction generated is a MOVPRFX"); } @@ -18485,8 +18485,8 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_BV_2B: // ., /M, #0.0 case IF_SVE_CQ_3A: // ., /M, // Tied registers - case IF_SVE_AM_2A: // ., /M, ., # - case IF_SVE_HM_2A: // ., /M, ., + case IF_SVE_AM_2A: // ., /M, ., # + case IF_SVE_HM_2A: // ., /M, ., assert(movprefxIsPredicated); break; @@ -18515,10 +18515,10 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) break; // Tied registers - case IF_SVE_AW_2A: // ., ., ., # - case IF_SVE_BY_2A: // .B, .B, .B, # - case IF_SVE_FV_2A: // ., ., ., - case IF_SVE_HN_2A: // ., ., ., # + case IF_SVE_AW_2A: // ., ., ., # + case IF_SVE_BY_2A: // .B, .B, .B, # + case IF_SVE_FV_2A: // ., ., ., + case IF_SVE_HN_2A: // ., ., ., # assert(!movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg3()); break; From bcc454fbe90c738e4d064e97a9158676422dd88c Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Tue, 13 Aug 2024 08:42:07 +0100 Subject: [PATCH 08/18] fixups --- src/coreclr/jit/emit.cpp | 4 ++-- src/coreclr/jit/emitarm64sve.cpp | 17 +++-------------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 91f3b40568e91f..28efb8231dbc7a 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -7110,10 +7110,10 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, prevId = currId; emitAdvanceInstrDesc(&currId, emitSizeOfInsDsc(currId)); } - // Additional check for the final instruction + // Final instruction can't be a movprfx if (ig->igNext == nullptr) { - emitInsPairSanityCheck(prevId, nullptr); + assert(prevId->idIns() != INS_sve_movprfx); } #endif // DEBUG && TARGET_ARM64 diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 4b9ebf0191e11c..80c7212bae1fc4 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18429,7 +18429,7 @@ void emitter::getInsSveExecutionCharacteristics(instrDesc* id, insExecutionChara void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) { - if (firstId == nullptr) + if (firstId == nullptr || secondId == nullptr) { return; } @@ -18440,12 +18440,6 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) return; } - // The movprfx can't be the last instruction - if (secondId == nullptr) - { - assert(!"Last instruction generated is a MOVPRFX"); - } - bool movprefxIsPredicated = false; if (firstId->idInsFmt() == IF_SVE_AH_3A) { @@ -18453,7 +18447,7 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) } else { - // 2 operand version + // Unpredicated version assert(firstId->idInsFmt() == IF_SVE_BI_2A); } @@ -18634,10 +18628,8 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) break; } - if (firstId->idInsFmt() == IF_SVE_AH_3A) + if (movprefxIsPredicated) { - // 3 operand version - // "The prefixed instruction must specify the same predicate register" assert(isPredicateRegister(firstId->idReg2())); assert(isPredicateRegister(secondId->idReg2())); @@ -18651,9 +18643,6 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) } else { - // 2 operand version - assert(firstId->idInsFmt() == IF_SVE_BI_2A); - // "The prefixed instruction must specify the same destination vector as the MOVPRFX instruction." assert(firstId->idReg1() == secondId->idReg1()); } From ebcdebc477b6aa327adb2726190cb516b2d06533 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Tue, 13 Aug 2024 08:43:18 +0100 Subject: [PATCH 09/18] Remove GenerateHWIntrinsicTests_Arm.cs changes --- .../GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs b/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs index a7e11b4fe30b9d..d5b3d81b24623d 100644 --- a/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs +++ b/src/tests/Common/GenerateHWIntrinsicTests/GenerateHWIntrinsicTests_Arm.cs @@ -3132,8 +3132,8 @@ ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_AddSaturate_uint", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSaturate", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt32", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt32", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "UInt32", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt32()", ["NextValueOp2"] = "TestLibrary.Generator.GetUInt32()", ["ValidateIterResult"] = "Helpers.AddSaturate(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.AddSaturate(left[i], right[i])"}), ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_AddSaturate_ulong", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSaturate", ["RetVectorType"] = "Vector", ["RetBaseType"] = "UInt64", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "UInt64", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "UInt64", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "TestLibrary.Generator.GetUInt64()", ["NextValueOp2"] = "TestLibrary.Generator.GetUInt64()", ["ValidateIterResult"] = "Helpers.AddSaturate(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.AddSaturate(left[i], right[i])"}), - // ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), - // ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Double", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskDouble()", ["NextValueOp2"] = "TestLibrary.Generator.GetDouble()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), + ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_float", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Single", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Single", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Single", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskSingle()", ["NextValueOp2"] = "TestLibrary.Generator.GetSingle()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), + ("SveVecBinOpTestScalarRet.template", new Dictionary { ["TestName"] = "Sve_AddSequentialAcross_double", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "AddSequentialAcross", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Double", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Double", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Double", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "Helpers.getMaskDouble()", ["NextValueOp2"] = "TestLibrary.Generator.GetDouble()", ["ValidateScalarResult"] = "result[0] != Helpers.AddSequentialAcross(left, right)"}), ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_And_sbyte", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "And", ["RetVectorType"] = "Vector", ["RetBaseType"] = "SByte", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "SByte", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "SByte", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "(sbyte)TestLibrary.Generator.GetSByte()", ["NextValueOp2"] = "TestLibrary.Generator.GetSByte()", ["ValidateIterResult"] = "Helpers.And(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.And(left[i], right[i])"}), ("SveVecBinOpTest.template", new Dictionary { ["TestName"] = "Sve_And_short", ["Isa"] = "Sve", ["LoadIsa"] = "Sve", ["Method"] = "And", ["RetVectorType"] = "Vector", ["RetBaseType"] = "Int16", ["Op1VectorType"] = "Vector", ["Op1BaseType"] = "Int16", ["Op2VectorType"] = "Vector", ["Op2BaseType"] = "Int16", ["LargestVectorSize"] = "64", ["NextValueOp1"] = "(short)TestLibrary.Generator.GetInt16()", ["NextValueOp2"] = "TestLibrary.Generator.GetInt16()", ["ValidateIterResult"] = "Helpers.And(left[i], right[i]) != result[i]", ["GetIterResult"] = "Helpers.And(left[i], right[i])"}), From 5752964a26645769732cf0e2a2d97a302d57abae Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Tue, 13 Aug 2024 10:14:02 +0100 Subject: [PATCH 10/18] move idReg1() check --- src/coreclr/jit/emitarm64sve.cpp | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 80c7212bae1fc4..c0ca5b9e067441 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18451,7 +18451,7 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) assert(firstId->idInsFmt() == IF_SVE_BI_2A); } - // Quoted sections are taken fron the Arm manual. + // Quoted sections are taken from the Arm manual. // "It is required that the prefixed instruction at PC+4 must be an SVE destructive binary or ternary // instruction encoding, or a unary operation with merging predication, but excluding other MOVPRFX instructions." @@ -18628,6 +18628,9 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) break; } + // "The prefixed instruction must specify the same destination vector as the MOVPRFX instruction." + assert(firstId->idReg1() == secondId->idReg1()); + if (movprefxIsPredicated) { // "The prefixed instruction must specify the same predicate register" @@ -18635,16 +18638,8 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) assert(isPredicateRegister(secondId->idReg2())); assert(firstId->idReg2() == secondId->idReg2()); - // "and have the same maximum element size (ignoring a fixed 64-bit "wide vector" operand)," + // "predicated using the same governing predicate register and source element size as this instruction." assert(firstId->idInsOpt() == secondId->idInsOpt()); - - // "and the same destination vector as the MOVPRFX instruction." - assert(firstId->idReg1() == secondId->idReg1()); - } - else - { - // "The prefixed instruction must specify the same destination vector as the MOVPRFX instruction." - assert(firstId->idReg1() == secondId->idReg1()); } } #endif // DEBUG From 52c7ef82fce6c04c906f014f5a6d58ef94115a4b Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 12:41:29 +0100 Subject: [PATCH 11/18] Allow unpredicated movprfx with predicated instruction --- src/coreclr/jit/emitarm64sve.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index ea35fac6b87370..a611f17ff6f164 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18468,6 +18468,7 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) // instruction encoding, or a unary operation with merging predication, but excluding other MOVPRFX instructions." // "The prefixed instruction must not use the destination register in any other operand position, even if // they have different names but refer to the same architectural register state." + // "A predicated MOVPRFX cannot be used with an unpredicated instruction." switch (secondId->idInsFmt()) { case IF_SVE_BN_1A: // .D{, {, MUL #}} @@ -18492,7 +18493,6 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) // Tied registers case IF_SVE_AM_2A: // ., /M, ., # case IF_SVE_HM_2A: // ., /M, ., - assert(movprefxIsPredicated); break; case IF_SVE_FU_2A: // ., ., # @@ -18515,7 +18515,6 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_HR_3A: // ., /M, . case IF_SVE_HS_3A: // ., /M, . case IF_SVE_HP_3B: // ., /M, . - assert(movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg3()); break; @@ -18594,7 +18593,6 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_GR_3A: // ., /M, ., . case IF_SVE_HL_3A: // ., /M, ., . case IF_SVE_HL_3B: // .H, /M, .H, .H - assert(movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg4()); break; @@ -18611,7 +18609,6 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_HU_4A: // ., /M, ., . case IF_SVE_HU_4B: // .H, /M, .H, .H case IF_SVE_HV_4A: // ., /M, ., . - assert(movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg3()); assert(secondId->idReg1() != secondId->idReg4()); break; From 29e8efd54901fd237a3ee16dcde4cd2ac4601f8e Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 14:39:21 +0100 Subject: [PATCH 12/18] Fix NI_Sve_ExtractVector --- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 2d44b1b1d69dba..2f4bc4cd0ef814 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -2408,17 +2408,17 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) { assert(isRMW); + if (targetReg != op1Reg) + { + assert(targetReg != op2Reg); + + GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, op1Reg, /* canSkip */ true); + } + HWIntrinsicImmOpHelper helper(this, intrin.op3, node); for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { - if (targetReg != op1Reg) - { - assert(targetReg != op2Reg); - - GetEmitter()->emitIns_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, op1Reg); - } - const int elementIndex = helper.ImmValue(); const int byteIndex = genTypeSize(intrin.baseType) * elementIndex; @@ -2560,7 +2560,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) HWIntrinsicImmOpHelper helper(this, intrin.op3, node); - for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) + for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd(), (targetReg != op1Reg) ? 2 : 1) { if (targetReg != op1Reg) { @@ -2610,7 +2610,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) // Generate the table using the combined immediate HWIntrinsicImmOpHelper helper(this, op4Reg, 0, 7, node); - for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) + for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd(), (targetReg != op1Reg) ? 2 : 1) { if (targetReg != op1Reg) { From 8c7e9d357b1bd6b1df2e1af7b7090a7ea61c5ca2 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 15:10:35 +0100 Subject: [PATCH 13/18] Fix for uses with zero masks (eg loads) --- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 2f4bc4cd0ef814..68b02004198a5d 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -649,6 +649,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) if (intrin.op3->isContained()) { assert(intrin.op3->IsVectorZero()); + if (intrin.op1->isContained() || intrin.op1->IsMaskAllBitsSet()) { // We already skip importing ConditionalSelect if op1 == trueAll, however @@ -659,6 +660,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) } else { + assert(!HWIntrinsicInfo::IsZeroingMaskedOperation(intrinEmbMask.id)); + // If falseValue is zero, just zero out those lanes of targetReg using `movprfx` // and /Z GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, emitSize, targetReg, maskReg, targetReg, @@ -680,6 +683,14 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) falseReg, opt); break; } + else if (HWIntrinsicInfo::IsZeroingMaskedOperation(intrinEmbMask.id)) + { + // At this point, target != embMaskOp1Reg != falseReg, so just go ahead + // and move the falseReg unpredicated into targetReg. + // Cannot use movprfx for zeroing mask operations. + GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, falseReg, + /* canSkip */ true); + } else { // At this point, target != embMaskOp1Reg != falseReg, so just go ahead From b9e46ae718b8294326daa9496cb94cf81e08de1a Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 15:21:19 +0100 Subject: [PATCH 14/18] Fix tied register checking --- src/coreclr/jit/emitarm64sve.cpp | 42 ++++++++++++-------------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index a611f17ff6f164..1c4bbda2abcb71 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18496,6 +18496,11 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) break; case IF_SVE_FU_2A: // ., ., # + // Tied registers + case IF_SVE_AW_2A: // ., ., ., # + case IF_SVE_BY_2A: // .B, .B, .B, # + case IF_SVE_FV_2A: // ., ., ., + case IF_SVE_HN_2A: // ., ., ., # assert(!movprefxIsPredicated); assert(secondId->idReg1() != secondId->idReg2()); break; @@ -18515,15 +18520,16 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_HR_3A: // ., /M, . case IF_SVE_HS_3A: // ., /M, . case IF_SVE_HP_3B: // ., /M, . - assert(secondId->idReg1() != secondId->idReg3()); - break; - // Tied registers - case IF_SVE_AW_2A: // ., ., ., # - case IF_SVE_BY_2A: // .B, .B, .B, # - case IF_SVE_FV_2A: // ., ., ., - case IF_SVE_HN_2A: // ., ., ., # - assert(!movprefxIsPredicated); + case IF_SVE_AA_3A: // ., /M, ., . + case IF_SVE_AB_3B: // .D, /M, .D, .D + case IF_SVE_AC_3A: // ., /M, ., . + case IF_SVE_AO_3A: // ., /M, ., .D + case IF_SVE_CM_3A: // ., , ., . + case IF_SVE_GP_3A: // ., /M, ., ., + case IF_SVE_GR_3A: // ., /M, ., . + case IF_SVE_HL_3A: // ., /M, ., . + case IF_SVE_HL_3B: // .H, /M, .H, .H assert(secondId->idReg1() != secondId->idReg3()); break; @@ -18578,29 +18584,11 @@ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) case IF_SVE_HC_3A: // .S, .B, .B[] case IF_SVE_HD_3A: // .S, .H, .H case IF_SVE_HD_3A_A: // .D, .D, .D - assert(!movprefxIsPredicated); - assert(secondId->idReg1() != secondId->idReg2()); - assert(secondId->idReg1() != secondId->idReg3()); - break; - - // Tied registers - case IF_SVE_AA_3A: // ., /M, ., . - case IF_SVE_AB_3B: // .D, /M, .D, .D - case IF_SVE_AC_3A: // ., /M, ., . - case IF_SVE_AO_3A: // ., /M, ., .D - case IF_SVE_CM_3A: // ., , ., . - case IF_SVE_GP_3A: // ., /M, ., ., - case IF_SVE_GR_3A: // ., /M, ., . - case IF_SVE_HL_3A: // ., /M, ., . - case IF_SVE_HL_3B: // .H, /M, .H, .H - assert(secondId->idReg1() != secondId->idReg4()); - break; - // Tied registers case IF_SVE_AV_3A: // .D, .D, .D, .D assert(!movprefxIsPredicated); + assert(secondId->idReg1() != secondId->idReg2()); assert(secondId->idReg1() != secondId->idReg3()); - assert(secondId->idReg1() != secondId->idReg4()); break; case IF_SVE_AR_4A: // ., /M, ., . From 430c86c0fb17da866aca4df6f3d7f71698be8f49 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 18:10:24 +0100 Subject: [PATCH 15/18] revert type of movprfx --- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 68b02004198a5d..86cd7814a0dfd2 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -774,14 +774,14 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) HWIntrinsicImmOpHelper helper(this, intrinEmbMask.op2, op2->AsHWIntrinsic(), 2); for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { - GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, reg1, reg2, reg3, opt); + GetEmitter()->emitIns_R_R(INS_sve_movprfx, EA_SCALABLE, reg1, reg3); GetEmitter()->emitInsSve_R_R_I(insEmbMask, emitSize, reg1, reg2, helper.ImmValue(), embOpt, sopt); } } else { - GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, reg1, reg2, reg3, opt); + GetEmitter()->emitIns_R_R(INS_sve_movprfx, EA_SCALABLE, reg1, reg3); GetEmitter()->emitIns_R_R_R(insEmbMask, emitSize, reg1, reg2, reg4, embOpt, sopt); } }; @@ -818,7 +818,21 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) // Finally, perform the actual "predicated" operation so that `targetReg` is the first // operand and `embMaskOp2Reg` is the second operand. - emitInsMovPrfxHelper(targetReg, maskReg, embMaskOp1Reg, embMaskOp2Reg); + if (hasShift) + { + HWIntrinsicImmOpHelper helper(this, intrinEmbMask.op2, op2->AsHWIntrinsic(), 2); + for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) + { + GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, maskReg, embMaskOp1Reg, opt); + GetEmitter()->emitInsSve_R_R_I(insEmbMask, emitSize, targetReg, maskReg, helper.ImmValue(), + embOpt, sopt); + } + } + else + { + GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, maskReg, embMaskOp1Reg, opt); + GetEmitter()->emitIns_R_R_R(insEmbMask, emitSize, targetReg, maskReg, embMaskOp2Reg, embOpt, sopt); + } break; } } @@ -856,6 +870,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) // embMaskOp1Reg is same as `falseReg`, but not same as `targetReg`. Move the // `embMaskOp1Reg` i.e. `falseReg` in `targetReg`, using "unpredicated movprfx", so the // subsequent `insEmbMask` operation can be merged on top of it. + emitInsMovPrfxHelper(targetReg, maskReg, falseReg, embMaskOp2Reg); } else From 8bee58f1e3dd9a8e1d45242c1d3cf573486c826a Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 18:14:23 +0100 Subject: [PATCH 16/18] fix comments --- src/coreclr/jit/emit.cpp | 4 ++-- src/coreclr/jit/emitarm64sve.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 28efb8231dbc7a..9afbc4b39d2d43 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -7097,7 +7097,7 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, #endif #if defined(DEBUG) && defined(TARGET_ARM64) instrDesc* prevId = nullptr; -#endif // DEBUG && TARGET_ARM64 +#endif // defined(DEBUG) && defined(TARGET_ARM64) for (insGroup* ig = emitIGlist; ig != nullptr; ig = ig->igNext) { @@ -7115,7 +7115,7 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, { assert(prevId->idIns() != INS_sve_movprfx); } -#endif // DEBUG && TARGET_ARM64 +#endif // defined(DEBUG) && defined(TARGET_ARM64) assert(!(ig->igFlags & IGF_PLACEHOLDER)); // There better not be any placeholder groups left diff --git a/src/coreclr/jit/emitarm64sve.cpp b/src/coreclr/jit/emitarm64sve.cpp index 1c4bbda2abcb71..72045b32e87ebe 100644 --- a/src/coreclr/jit/emitarm64sve.cpp +++ b/src/coreclr/jit/emitarm64sve.cpp @@ -18435,7 +18435,7 @@ void emitter::getInsSveExecutionCharacteristics(instrDesc* id, insExecutionChara #ifdef DEBUG /***************************************************************************** * - * Sanity check two instruction are valid when placed next to each other + * Sanity check two instructions are valid when placed next to each other */ void emitter::emitInsPairSanityCheck(instrDesc* firstId, instrDesc* secondId) From 4f3610dde1f0ee1b92678fbcbf5f965048456c62 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 18:18:37 +0100 Subject: [PATCH 17/18] Fix use of immediate helpers --- src/coreclr/jit/codegen.h | 2 +- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 252d5bab813631..938f56e904c28f 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1013,7 +1013,7 @@ class CodeGen final : public CodeGenInterface HWIntrinsicImmOpHelper(CodeGen* codeGen, GenTree* immOp, GenTreeHWIntrinsic* intrin, int numInstrs = 1); HWIntrinsicImmOpHelper( - CodeGen* codeGen, regNumber immReg, int immLowerBound, int immUpperBound, GenTreeHWIntrinsic* intrin); + CodeGen* codeGen, regNumber immReg, int immLowerBound, int immUpperBound, GenTreeHWIntrinsic* intrin, int numInstrs = 1); void EmitBegin(); void EmitCaseEnd(); diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 86cd7814a0dfd2..1e73ad3f363493 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -128,7 +128,7 @@ CodeGen::HWIntrinsicImmOpHelper::HWIntrinsicImmOpHelper(CodeGen* code // Note: This instance is designed to be used via the same for loop as the standard constructor. // CodeGen::HWIntrinsicImmOpHelper::HWIntrinsicImmOpHelper( - CodeGen* codeGen, regNumber immReg, int immLowerBound, int immUpperBound, GenTreeHWIntrinsic* intrin) + CodeGen* codeGen, regNumber immReg, int immLowerBound, int immUpperBound, GenTreeHWIntrinsic* intrin, int numInstrs) : codeGen(codeGen) , endLabel(nullptr) , nonZeroLabel(nullptr) @@ -137,7 +137,7 @@ CodeGen::HWIntrinsicImmOpHelper::HWIntrinsicImmOpHelper( , immUpperBound(immUpperBound) , nonConstImmReg(immReg) , branchTargetReg(REG_NA) - , numInstrs(1) + , numInstrs(numInstrs) { assert(codeGen != nullptr); @@ -2584,9 +2584,9 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) { assert(isRMW); - HWIntrinsicImmOpHelper helper(this, intrin.op3, node); + HWIntrinsicImmOpHelper helper(this, intrin.op3, node, (targetReg != op1Reg) ? 2 : 1); - for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd(), (targetReg != op1Reg) ? 2 : 1) + for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { if (targetReg != op1Reg) { @@ -2635,8 +2635,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) GetEmitter()->emitIns_R_R_R(INS_orr, scalarSize, op4Reg, op4Reg, op5Reg); // Generate the table using the combined immediate - HWIntrinsicImmOpHelper helper(this, op4Reg, 0, 7, node); - for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd(), (targetReg != op1Reg) ? 2 : 1) + HWIntrinsicImmOpHelper helper(this, op4Reg, 0, 7, node, (targetReg != op1Reg) ? 2 : 1); + for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { if (targetReg != op1Reg) { From 190026b39e7e9320e835ac19b25e715abf9afae3 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 14 Aug 2024 18:44:56 +0100 Subject: [PATCH 18/18] fix formatting --- src/coreclr/jit/codegen.h | 8 ++++++-- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 13 ++++++++----- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 938f56e904c28f..1e53616c2d8e7a 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1012,8 +1012,12 @@ class CodeGen final : public CodeGenInterface public: HWIntrinsicImmOpHelper(CodeGen* codeGen, GenTree* immOp, GenTreeHWIntrinsic* intrin, int numInstrs = 1); - HWIntrinsicImmOpHelper( - CodeGen* codeGen, regNumber immReg, int immLowerBound, int immUpperBound, GenTreeHWIntrinsic* intrin, int numInstrs = 1); + HWIntrinsicImmOpHelper(CodeGen* codeGen, + regNumber immReg, + int immLowerBound, + int immUpperBound, + GenTreeHWIntrinsic* intrin, + int numInstrs = 1); void EmitBegin(); void EmitCaseEnd(); diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 1e73ad3f363493..1f884807a5b415 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -823,15 +823,18 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) HWIntrinsicImmOpHelper helper(this, intrinEmbMask.op2, op2->AsHWIntrinsic(), 2); for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { - GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, maskReg, embMaskOp1Reg, opt); - GetEmitter()->emitInsSve_R_R_I(insEmbMask, emitSize, targetReg, maskReg, helper.ImmValue(), - embOpt, sopt); + GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, maskReg, + embMaskOp1Reg, opt); + GetEmitter()->emitInsSve_R_R_I(insEmbMask, emitSize, targetReg, maskReg, + helper.ImmValue(), embOpt, sopt); } } else { - GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, maskReg, embMaskOp1Reg, opt); - GetEmitter()->emitIns_R_R_R(insEmbMask, emitSize, targetReg, maskReg, embMaskOp2Reg, embOpt, sopt); + GetEmitter()->emitIns_R_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, maskReg, + embMaskOp1Reg, opt); + GetEmitter()->emitIns_R_R_R(insEmbMask, emitSize, targetReg, maskReg, embMaskOp2Reg, + embOpt, sopt); } break; }