From 03f1b30afeb4133301495e5e179e7b718954818c Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 3 Sep 2024 18:14:57 -0400 Subject: [PATCH 1/7] Don't emit mov for zero shift amount --- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 5bf021ac90b55..32492363d548a 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -443,18 +443,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { const int shiftAmount = helper.ImmValue(); - - if (shiftAmount == 0) - { - // TODO: Use emitIns_Mov instead. - // We do not use it currently because it will still elide the 'mov' - // even if 'canSkip' is false. We cannot elide the 'mov' here. - GetEmitter()->emitIns_R_R_R(INS_mov, emitTypeSize(node), targetReg, reg, reg); - } - else - { - GetEmitter()->emitIns_R_R_I(ins, emitSize, targetReg, reg, shiftAmount, opt); - } + assert((shiftAmount != 0) || (intrin.category == HW_Category_ShiftLeftByImmediate)); + GetEmitter()->emitIns_R_R_I(ins, emitSize, targetReg, reg, shiftAmount, opt); } }; From 1a6c7762e28241e3007e114fa39fee420cef8e8c Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 3 Sep 2024 18:25:18 -0400 Subject: [PATCH 2/7] Add test --- .../JitBlue/Runtime_107173/Runtime_107173.cs | 38 +++++++++++++++++++ .../Runtime_107173/Runtime_107173.csproj | 8 ++++ 2 files changed, 46 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.csproj diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs b/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs new file mode 100644 index 0000000000000..0bec1543d8cd6 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs @@ -0,0 +1,38 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Generated by Fuzzlyn v2.4 on 2024-08-26 23:38:13 +// Run on Arm64 Linux +// Seed: 8716802894387291290-vectort,vector64,vector128,armadvsimd,armadvsimdarm64,armaes,armarmbase,armarmbasearm64,armcrc32,armcrc32arm64,armdp,armrdm,armrdmarm64,armsha1,armsha256 +// Reduced from 19.5 KiB to 0.5 KiB in 00:00:27 +// Debug: Outputs <0, 0, 0, 0> +// Release: Outputs <0, 0, 4457472, 0> +using System; +using System.Numerics; +using System.Runtime.Intrinsics; +using System.Runtime.Intrinsics.Arm; +using Xunit; + +public class C0 +{ + public ushort F2; + public ushort F8; +} + +public class Runtime_107173 +{ + public static C0 s_8 = new C0(); + + [Fact] + public static void TestEntryPoint() + { + if (AdvSimd.IsSupported) + { + var vr6 = s_8.F8; + var vr7 = s_8.F2; + var vr8 = Vector64.Create(vr6, vr7, 0, 0); + Vector128 vr9 = AdvSimd.ShiftLeftLogicalWideningLower(vr8, 0); + Assert.Equal(vr9, Vector128.Zero); + } + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.csproj new file mode 100644 index 0000000000000..de6d5e08882e8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.csproj @@ -0,0 +1,8 @@ + + + True + + + + + From 66a8dcc557c61bf12693d3d3f6cf1a60c022795d Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 3 Sep 2024 21:03:08 -0400 Subject: [PATCH 3/7] Revert "Don't emit mov for zero shift amount" This reverts commit cd2656fcca259e9cec56afcf5303f2b7237a9e8a. --- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 32492363d548a..5bf021ac90b55 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -443,8 +443,18 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { const int shiftAmount = helper.ImmValue(); - assert((shiftAmount != 0) || (intrin.category == HW_Category_ShiftLeftByImmediate)); - GetEmitter()->emitIns_R_R_I(ins, emitSize, targetReg, reg, shiftAmount, opt); + + if (shiftAmount == 0) + { + // TODO: Use emitIns_Mov instead. + // We do not use it currently because it will still elide the 'mov' + // even if 'canSkip' is false. We cannot elide the 'mov' here. + GetEmitter()->emitIns_R_R_R(INS_mov, emitTypeSize(node), targetReg, reg, reg); + } + else + { + GetEmitter()->emitIns_R_R_I(ins, emitSize, targetReg, reg, shiftAmount, opt); + } } }; From 850cf0e03f5fbb153e58e5d2c80f69f7fa53f7fa Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 3 Sep 2024 21:21:44 -0400 Subject: [PATCH 4/7] Don't emit mov for zero left-shift amount --- src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index 5bf021ac90b55..bfea98a4e7bd7 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -444,12 +444,15 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) { const int shiftAmount = helper.ImmValue(); - if (shiftAmount == 0) + if ((shiftAmount == 0) && (intrin.category == HW_Category_ShiftRightByImmediate)) { - // TODO: Use emitIns_Mov instead. - // We do not use it currently because it will still elide the 'mov' - // even if 'canSkip' is false. We cannot elide the 'mov' here. - GetEmitter()->emitIns_R_R_R(INS_mov, emitTypeSize(node), targetReg, reg, reg); + // For operations like `Vector128 >> 8`, over-shifting behavior will mask + // the immediate so that it's zero, and the shift will be imported as a GT_HWINTRINSIC. + // If this isn't const-folded away, it is possible to try to emit a right-shift by zero, + // but only if the immediate is known (we won't emit a zero case for the jump table). + // Since we cannot encode a right-shift by 0 on ARM64, emit a mov instead. + assert(op->isContainedIntOrIImmed()); + GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, reg, /* canSkip */ true); } else { From 0e18c1b39e248b842f14acb8e147b842ce841358 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Tue, 3 Sep 2024 21:27:56 -0400 Subject: [PATCH 5/7] Update test --- .../Regression/JitBlue/Runtime_107173/Runtime_107173.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs b/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs index 0bec1543d8cd6..147b25e47c49d 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_107173/Runtime_107173.cs @@ -24,7 +24,7 @@ public class Runtime_107173 public static C0 s_8 = new C0(); [Fact] - public static void TestEntryPoint() + public static void TestLeftShift() { if (AdvSimd.IsSupported) { @@ -35,4 +35,11 @@ public static void TestEntryPoint() Assert.Equal(vr9, Vector128.Zero); } } + + [Fact] + public static void TestRightShift() + { + var result = Vector128.AllBitsSet >> 8; + Assert.Equal(result, Vector128.AllBitsSet); + } } \ No newline at end of file From eec94334227ac7151cdf6d766f6f8090307238b2 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Thu, 5 Sep 2024 18:04:19 -0400 Subject: [PATCH 6/7] Use fallback intrinsic for >> by zero; remove mov special case --- src/coreclr/jit/gentree.cpp | 10 ++++++++++ src/coreclr/jit/hwintrinsiccodegenarm64.cpp | 17 ++--------------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index adb85ddda13a7..ae8cf3b777613 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -20818,6 +20818,16 @@ GenTree* Compiler::gtNewSimdBinOpNode( if (op2->IsCnsIntOrI()) { op2->AsIntCon()->gtIconVal &= shiftCountMask; +#ifdef TARGET_ARM64 + // On ARM64, ShiftRight* intrinsics cannot encode a shift value of zero, + // so use the generic Shift* fallback intrinsic. + // GenTreeHWIntrinsic::GetHWIntrinsicIdForBinOp will see that the immediate node is not const, + // and return the correct fallback intrinsic. + if ((op != GT_LSH) && (op2->AsIntCon()->IconValue() == 0)) + { + op2 = gtNewSimdCreateBroadcastNode(type, op2, simdBaseJitType, simdSize); + } +#endif // TARGET_ARM64 } else { diff --git a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp index bfea98a4e7bd7..32492363d548a 100644 --- a/src/coreclr/jit/hwintrinsiccodegenarm64.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenarm64.cpp @@ -443,21 +443,8 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) for (helper.EmitBegin(); !helper.Done(); helper.EmitCaseEnd()) { const int shiftAmount = helper.ImmValue(); - - if ((shiftAmount == 0) && (intrin.category == HW_Category_ShiftRightByImmediate)) - { - // For operations like `Vector128 >> 8`, over-shifting behavior will mask - // the immediate so that it's zero, and the shift will be imported as a GT_HWINTRINSIC. - // If this isn't const-folded away, it is possible to try to emit a right-shift by zero, - // but only if the immediate is known (we won't emit a zero case for the jump table). - // Since we cannot encode a right-shift by 0 on ARM64, emit a mov instead. - assert(op->isContainedIntOrIImmed()); - GetEmitter()->emitIns_Mov(INS_mov, emitTypeSize(node), targetReg, reg, /* canSkip */ true); - } - else - { - GetEmitter()->emitIns_R_R_I(ins, emitSize, targetReg, reg, shiftAmount, opt); - } + assert((shiftAmount != 0) || (intrin.category == HW_Category_ShiftLeftByImmediate)); + GetEmitter()->emitIns_R_R_I(ins, emitSize, targetReg, reg, shiftAmount, opt); } }; From 2f4464be1d767f887f946b77cb90c563d8775805 Mon Sep 17 00:00:00 2001 From: "Aman Khalid (from Dev Box)" Date: Fri, 6 Sep 2024 15:01:00 -0400 Subject: [PATCH 7/7] Feedback --- src/coreclr/jit/gentree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index ae8cf3b777613..0142eb9481b03 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -20825,7 +20825,7 @@ GenTree* Compiler::gtNewSimdBinOpNode( // and return the correct fallback intrinsic. if ((op != GT_LSH) && (op2->AsIntCon()->IconValue() == 0)) { - op2 = gtNewSimdCreateBroadcastNode(type, op2, simdBaseJitType, simdSize); + op2 = gtNewZeroConNode(type); } #endif // TARGET_ARM64 }