This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
JIT: Fix value type box optimization (#13016)
Boxing a value type produces a non-null result. If the result of the box is only used to feed a compare against null, the jit tries to optimize the box away entirely since the result of the comparison is known. Such idiomatic expressions arise fairly often in generics instantiated over value types. In the current implementation the box expands into two parts. The first is an upstream statement to allocate a boxed object and assign a reference to the boxed object to a local var known as the "box temp". The second is an expression tree whose value is the box temp that also contains an an encapsulated copy from the value being boxed to the payload section of the boxed object. The box node also contains a pointer back to the first statement (more on this later). In the examples being discussed here this second tree is a child of a compare node whose other child is a null pointer. When the optimization fires, the upstream allocation statement is located via the pointer in the box node and removed, and the entire compare is replaced with a constant 0 or 1 as appropriate. Unfortunately the encapsulated copy in the box subtree may include side effects that should be preserved, and so this transformation is unsafe. Note that the copy subtree as a whole will always contain side effects, since the copy is storing values into the heap, and that copy now will not happen. But the side effects that happen when producing the value to box must remain. In the initial example from #12949 the side effects in question were introduced by the jit's optimizer to capure a CSE definition. #13016 gives several other examples where the side effects are present in the initial user code. For instance the value being boxed might come from an array, in which case the encapsulated copy in the box expression tree would contain the array null check and bounds check. So removing the entire tree can alter behavior. This fix attempts to carefully preserve the important side effects by reworking how a box is imported. The copy is now moved out from under the box into a second upstream statement. The box itself is then just a trivial side-effect-free reference to the box temp. To ensure proper ordering of side effects the jit spills the evaluation stack before appending the copy statement. When the optimization fires the jit removes the upstream heap allocation as before, as well as the now-trivial compare tree. It analyzes the source side of the upstream copy. If it is side effect free, the copy is removed entirely. If not, the jit modifies the copy into a minimal load of the boxed value, and this load should reproduce the necessary side effects. The optimization is only performed when the tree shape of the copy matches expected patterns. There are some expected cases where the tree won't match, for instance if the optimization is invoked while the jit is inlining. Because this optimization runs at several points the jit can catch these cases once inlining completes. There is one case that is not handled that could be -- if the assignment part of the copy is itself a subtree of a comma. This doesn't happen often. The optimization is now also extended to handle the case where the comparision operation is `cgt.un`. This doesn't catch any new cases but causes the optimization to happen earlier, typically during importation, which should reduce jit time slightly. Generally the split of the box into two upstream statements reduces code size, especially when the box expression is incorporated into a larger tree -- for example a call. However in some cases where the value being boxed comes from an array, preserving the array bounds check now causes loop cloning to kick in and increase code size. Hence the overall size impact on the jit-diff set is essentially zero. Added a number of new test cases showing the variety of situations that must be handled and the need to spill before appending the copy statement. Fixes #12949.
- Loading branch information
1 parent
6b38dca
commit 9c844a9
Showing
19 changed files
with
902 additions
and
19 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
51 changes: 51 additions & 0 deletions
51
tests/src/JIT/Regression/JitBlue/GitHub_12949/GitHub_12949_1.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // 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; | ||
|
|
||
| public struct S<K> | ||
| { | ||
| public int x; | ||
| public int y; | ||
| public K val; | ||
| } | ||
|
|
||
| public class X<K,V> | ||
| { | ||
| public X(K k) | ||
| { | ||
| a = new S<K>[2]; | ||
| a[1].val = k; | ||
| a[1].x = 3; | ||
| a[1].y = 4; | ||
| } | ||
|
|
||
| public void Assert(bool b) | ||
| { | ||
| if (!b) throw new Exception("bad!"); | ||
| } | ||
|
|
||
| public int Test() | ||
| { | ||
| int r = 0; | ||
| for (int i = 0; i < a.Length; i++) | ||
| { | ||
| Assert(a[i].val != null); | ||
| r += a[i].val.GetHashCode(); | ||
| } | ||
| return r; | ||
| } | ||
|
|
||
| S<K>[] a; | ||
| } | ||
|
|
||
| class B | ||
| { | ||
| public static int Main() | ||
| { | ||
| var a = new X<int, string>(11); | ||
| int z = a.Test(); | ||
| return (z == 11 ? 100 : 0); | ||
| } | ||
| } |
43 changes: 43 additions & 0 deletions
43
tests/src/JIT/Regression/JitBlue/GitHub_12949/GitHub_12949_1.csproj
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" /> | ||
| <PropertyGroup> | ||
| <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration> | ||
| <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform> | ||
| <AssemblyName>$(MSBuildProjectName)</AssemblyName> | ||
| <SchemaVersion>2.0</SchemaVersion> | ||
| <ProjectGuid>{7B521917-193E-48BB-86C6-FE013F3DFF35}</ProjectGuid> | ||
| <OutputType>Exe</OutputType> | ||
| <AppDesignerFolder>Properties</AppDesignerFolder> | ||
| <FileAlignment>512</FileAlignment> | ||
| <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids> | ||
| <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages</ReferencePath> | ||
| <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir> | ||
|
|
||
| <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp> | ||
| </PropertyGroup> | ||
| <!-- Default configurations to help VS understand the configurations --> | ||
| <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "> | ||
| </PropertyGroup> | ||
| <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies"> | ||
| <Visible>False</Visible> | ||
| </CodeAnalysisDependentAssemblyPaths> | ||
| </ItemGroup> | ||
| <PropertyGroup> | ||
| <DebugType></DebugType> | ||
| <Optimize>True</Optimize> | ||
| <AllowUnsafeBlocks>True</AllowUnsafeBlocks> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="$(MSBuildProjectName).cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup> | ||
| <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" /> | ||
| </ItemGroup> | ||
| <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" /> | ||
| <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "> | ||
| </PropertyGroup> | ||
| </Project> |
Oops, something went wrong.