From 993b7c5d4c002ff2f9f11b4ac3176baf72ec1b77 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Tue, 7 May 2019 15:35:42 -0700 Subject: Mark local struct as having overlapping fields after struct reinterpretation Methods like System.Runtime.CompilerServices.Unsafe.As may have struct reinterpretation when function signature specifies Struct1& and the method returns Struct2& where Struct1 and Struct2 are different structs. This may confuse jit optimizations (in particular, value numbering) because fields of a struct of type Struct1 may be accessed using handles of Struct2. This fix marks the source local involved in such struct reinterpretation as having overlapping fields. That prevents SSA builder from inserting the local into SSA. Fixes #24159. No diffs in framework assemblies and coreclr benchmarks. --- src/jit/importer.cpp | 40 +++++++++++++- tests/scripts/run-corefx-tests-exclusions.txt | 4 -- .../JitBlue/GitHub_24159/GitHub_24159.cs | 61 ++++++++++++++++++++++ .../JitBlue/GitHub_24159/GitHub_24159.csproj | 17 ++++++ 4 files changed, 117 insertions(+), 5 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.cs create mode 100644 tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.csproj diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 05eb086fdf..993889d0db 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -16428,7 +16428,6 @@ bool Compiler::impReturnInstruction(BasicBlock* block, int prefixFlags, OPCODE& } op2 = tmpOp2; - #ifdef DEBUG if (impInlineInfo->retExpr) { @@ -16440,6 +16439,45 @@ bool Compiler::impReturnInstruction(BasicBlock* block, int prefixFlags, OPCODE& #endif } + // If we are inlining a method that returns a struct byref, check whether we are "reinterpreting" the + // struct. + GenTree* effectiveRetVal = op2->gtEffectiveVal(); + if ((returnType == TYP_BYREF) && (info.compRetType == TYP_BYREF) && + (effectiveRetVal->OperGet() == GT_ADDR)) + { + GenTree* addrChild = effectiveRetVal->gtGetOp1(); + if (addrChild->OperGet() == GT_LCL_VAR) + { + LclVarDsc* varDsc = lvaGetDesc(addrChild->AsLclVarCommon()); + + if (varTypeIsStruct(addrChild->TypeGet()) && !isOpaqueSIMDLclVar(varDsc)) + { + CORINFO_CLASS_HANDLE referentClassHandle; + CorInfoType referentType = + info.compCompHnd->getChildType(info.compMethodInfo->args.retTypeClass, + &referentClassHandle); + if (varTypeIsStruct(JITtype2varType(referentType)) && + (varDsc->lvVerTypeInfo.GetClassHandle() != referentClassHandle)) + { + // We are returning a byref to struct1; the method signature specifies return type as + // byref + // to struct2. struct1 and struct2 are different so we are "reinterpreting" the struct. + // This may happen in, for example, System.Runtime.CompilerServices.Unsafe.As. + // We need to mark the source struct variable as having overlapping fields because its + // fields may be accessed using field handles of a different type, which may confuse + // optimizations, in particular, value numbering. + + JITDUMP("\nSetting lvOverlappingFields to true on V%02u because of struct " + "reinterpretation\n", + addrChild->AsLclVarCommon()->gtLclNum); + + varDsc->lvOverlappingFields = true; + } + } + } + } + #ifdef DEBUG if (verbose) { diff --git a/tests/scripts/run-corefx-tests-exclusions.txt b/tests/scripts/run-corefx-tests-exclusions.txt index 7027caa833..0c852dcf68 100644 --- a/tests/scripts/run-corefx-tests-exclusions.txt +++ b/tests/scripts/run-corefx-tests-exclusions.txt @@ -20,10 +20,6 @@ # # Please list a GitHub issue for every exclusion. -# https://github.com/dotnet/coreclr/issues/24159 --nomethod "System.Buffers.Tests.ArrayBufferWriterTests_Byte.Advance" --nomethod "System.Buffers.Tests.ArrayBufferWriterTests_Char.Advance" - # https://github.com/dotnet/coreclr/issues/22442 -nomethod "Microsoft.Win32.SystemEventsTests.CreateTimerTests.ConcurrentTimers" -nomethod "Microsoft.Win32.SystemEventsTests.SessionSwitchTests.SignalsSessionSwitch" diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.cs b/tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.cs new file mode 100644 index 0000000000..8017ca5e0c --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.cs @@ -0,0 +1,61 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +namespace GitHub_24159 +{ + + public struct Str1 + { + public int i1; + public int i2; + public int i3; + public int i4; + public int i5; + } + + public struct Str2 + { + public int j1; + public int j2; + public int j3; + public int j4; + public int j5; + } + + class Test + { + static int i; + + public static int Main() + { + i = 0; + + Str1 str1 = new Str1(); + + if (i != 0) + { + str1 = new Str1(); + } + else + { + str1.i2 = 7; + } + + // This call reinterprets the struct. + Str2 str2 = Unsafe.As(ref str1); + + // The bug was that value numbering couldn't recognize + // that this field has been updated on str1. + int k = str2.j2; + + Console.WriteLine(k); + + return k + 93; + + } + } +} diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.csproj new file mode 100644 index 0000000000..d86ed9f3d7 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/GitHub_24159/GitHub_24159.csproj @@ -0,0 +1,17 @@ + + + + + Release + AnyCPU + $(MSBuildProjectName) + Exe + + True + + + + + + + \ No newline at end of file -- cgit v1.2.3