From 7e030a88f7671b1d7893464b415c09e6c534be1b Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 25 Jun 2021 07:22:37 +0300 Subject: [PATCH 1/2] Always sign-extend from int32 in gtFoldExprConst GT_CNS_INT of TYP_INT on 64 hosts has an implicit contract: the value returned by IconValue() must "fit" into 32 bit signed integer, i. e. "static_cast(IconValue()) == IconValue()". gtFoldExprConst was failing to uphold this contract when the target was 32 bit and the host was 64 bit. Fix this by always truncating before calling SetIconValue(). --- src/coreclr/jit/gentree.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 1f37457ff5390..7aa9e7bdc5863 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -14864,19 +14864,15 @@ GenTree* Compiler::gtFoldExprConst(GenTree* tree) JITDUMP("\nFolding operator with constant nodes into a constant:\n"); DISPTREE(tree); -#ifdef TARGET_64BIT - // Some operations are performed as 64 bit instead of 32 bit so the upper 32 bits - // need to be discarded. Since constant values are stored as ssize_t and the node - // has TYP_INT the result needs to be sign extended rather than zero extended. - i1 = INT32(i1); -#endif // TARGET_64BIT - // Also all conditional folding jumps here since the node hanging from // GT_JTRUE has to be a GT_CNS_INT - value 0 or 1. tree->ChangeOperConst(GT_CNS_INT); tree->ChangeType(TYP_INT); - tree->AsIntCon()->SetIconValue(i1); + // Some operations are performed as 64 bit instead of 32 bit so the upper 32 bits + // need to be discarded. Since constant values are stored as ssize_t and the node + // has TYP_INT the result needs to be sign extended rather than zero extended. + tree->AsIntCon()->SetIconValue(static_cast(i1)); tree->AsIntCon()->gtFieldSeq = fieldSeq; if (vnStore != nullptr) { From 576315f3e8f8df2242b0382f52d42da8da16ec88 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 25 Jun 2021 07:48:35 +0300 Subject: [PATCH 2/2] Add a simple test that reproduces bad codegen --- .../folding_extends_int32_on_64_bit_hosts.cs | 27 +++++++++++++++++++ ...lding_extends_int32_on_64_bit_hosts.csproj | 10 +++++++ 2 files changed, 37 insertions(+) create mode 100644 src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.cs create mode 100644 src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.csproj diff --git a/src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.cs b/src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.cs new file mode 100644 index 0000000000000..08d7e7a684f7d --- /dev/null +++ b/src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.cs @@ -0,0 +1,27 @@ +public class FoldingExtendsInt32On64BitHostsTest +{ + // On 64 bit hosts, 32 bit constants are stored as 64 bit signed values. + // gtFoldExpr failed to properly truncate the folded value to 32 bits when + // the host was 64 bit and the target - 32 bit. Thus local assertion prop + // got the "poisoned" value, which lead to silent bad codegen. + + public static int Main() + { + var r1 = 31; + // "Poisoned" value. + var s1 = 0b11 << r1; + + if (s1 == 0b11 << 31) + { + return 100; + } + + // Just so that Roslyn actually uses locals. + Use(s1); + Use(r1); + + return -1; + } + + private static void Use(int a) { } +} diff --git a/src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.csproj b/src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.csproj new file mode 100644 index 0000000000000..edc51be9ca25b --- /dev/null +++ b/src/tests/JIT/Directed/ConstantFolding/folding_extends_int32_on_64_bit_hosts.csproj @@ -0,0 +1,10 @@ + + + Exe + True + None + + + + +