From 250b32965fdf798cc582f04010224c4dd889fa48 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 8 Apr 2022 00:39:03 +0200 Subject: [PATCH 1/2] Fix wrong constant folding for bswap16 The semantics of bswap16 is currently to swap the lower 2 bytes and leave the upper bytes alone. We need to keep the same behavior when constant folding or we can quickly end up discarding necessary casts. Fix #67723 --- src/coreclr/jit/gentree.cpp | 5 ++++- src/coreclr/jit/valuenum.cpp | 3 ++- .../JitBlue/Runtime_67723/Runtime_67723.cs | 15 +++++++++++++++ .../JitBlue/Runtime_67723/Runtime_67723.csproj | 9 +++++++++ 4 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.csproj diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d111ae7ed3f4f2..e80881aa0d3f3f 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -13440,7 +13440,10 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) break; case GT_BSWAP16: - i1 = ((i1 >> 8) & 0xFF) | ((i1 << 8) & 0xFF00); + // Semantics of bswap16 is to swap lower 2 bytes, but + // leaver upper 2 bytes alone (with a cast inserted as + // necessary during import). + i1 = (i1 & 0xFFFF0000) | ((i1 >> 8) & 0xFF) | ((i1 << 8) & 0xFF00); break; case GT_CAST: diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 27d00595522416..3348669df41eb8 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -562,7 +562,8 @@ T ValueNumStore::EvalOpSpecialized(VNFunc vnf, T v0) UINT16 v0_unsigned = UINT16(v0); v0_unsigned = ((v0_unsigned >> 8) & 0xFF) | ((v0_unsigned << 8) & 0xFF00); - return T(v0_unsigned); + // Swap 2 lower bytes, leave rest alone + return (v0 & ~T(0xFFFF)) | T(v0_unsigned); } case GT_BSWAP: diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.cs b/src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.cs new file mode 100644 index 00000000000000..24e3ecf0e311f8 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.cs @@ -0,0 +1,15 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Buffers.Binary; + +public class Runtime_67223 +{ + public static int Main() + { + short[] foo = { short.MinValue }; + int test = BinaryPrimitives.ReverseEndianness(foo[0]); + return test == 0x80 ? 100 : -1; + } +} + diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.csproj new file mode 100644 index 00000000000000..f492aeac9d056b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_67723/Runtime_67723.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + \ No newline at end of file From e955a8ccd73e70d7a6b7e68679615828c078b41f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 8 Apr 2022 01:11:01 +0200 Subject: [PATCH 2/2] Disable constant folding for bswap16 --- src/coreclr/jit/gentree.cpp | 10 +++------- src/coreclr/jit/gtlist.h | 2 +- src/coreclr/jit/valuenum.cpp | 14 ++++---------- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index e80881aa0d3f3f..d2341d16568a3a 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -13434,18 +13434,14 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) i1 = -i1; break; + // Note: BSWAP16 is considered to leave upper 16 bits + // undefined as it differs by platform, so we cannot + // constant fold it. case GT_BSWAP: i1 = ((i1 >> 24) & 0xFF) | ((i1 >> 8) & 0xFF00) | ((i1 << 8) & 0xFF0000) | ((i1 << 24) & 0xFF000000); break; - case GT_BSWAP16: - // Semantics of bswap16 is to swap lower 2 bytes, but - // leaver upper 2 bytes alone (with a cast inserted as - // necessary during import). - i1 = (i1 & 0xFFFF0000) | ((i1 >> 8) & 0xFF) | ((i1 << 8) & 0xFF00); - break; - case GT_CAST: // assert (genActualType(tree->CastToType()) == tree->TypeGet()); diff --git a/src/coreclr/jit/gtlist.h b/src/coreclr/jit/gtlist.h index c902b21ec72f47..2b52b81aa841fe 100644 --- a/src/coreclr/jit/gtlist.h +++ b/src/coreclr/jit/gtlist.h @@ -104,7 +104,7 @@ GTNODE(RUNTIMELOOKUP , GenTreeRuntimeLookup, 0,GTK_UNOP|GTK_EXOP|DBK_NOTLIR) GTNODE(ARR_ADDR , GenTreeArrAddr ,0,GTK_UNOP|GTK_EXOP|DBK_NOTLIR) // Wraps an array address expression GTNODE(BSWAP , GenTreeOp ,0,GTK_UNOP) // Byte swap (32-bit or 64-bit) -GTNODE(BSWAP16 , GenTreeOp ,0,GTK_UNOP) // Byte swap (16-bit) +GTNODE(BSWAP16 , GenTreeOp ,0,GTK_UNOP) // Byte swap lower 16 bits. The upper 16 bits are undefined. //----------------------------------------------------------------------------- // Binary operators (2 operands): diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 3348669df41eb8..c7f3200a5435ac 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -557,15 +557,9 @@ T ValueNumStore::EvalOpSpecialized(VNFunc vnf, T v0) case GT_NOT: return ~v0; - case GT_BSWAP16: - { - UINT16 v0_unsigned = UINT16(v0); - - v0_unsigned = ((v0_unsigned >> 8) & 0xFF) | ((v0_unsigned << 8) & 0xFF00); - // Swap 2 lower bytes, leave rest alone - return (v0 & ~T(0xFFFF)) | T(v0_unsigned); - } - + // Note: BSWAP16 is considered to leave upper 16 bits + // undefined as it differs by platform, so we cannot + // constant fold it. case GT_BSWAP: if (sizeof(T) == 4) { @@ -3356,7 +3350,7 @@ bool ValueNumStore::CanEvalForConstantArgs(VNFunc vnf) // Unary Ops case GT_NEG: case GT_NOT: - case GT_BSWAP16: + // Cannot constant fold BSWAP16 as upper bits are undefined. case GT_BSWAP: // Binary Ops