Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix spill check for struct lclVars #23570

Merged
merged 2 commits into from
Apr 2, 2019
Merged

Fix spill check for struct lclVars #23570

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07dcc8b
Fix spill check for struct lclVars
CarolEidt Mar 29, 2019
e0ef393
Simplify the check for local spill, add comments, and delete redundan…
CarolEidt Apr 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions src/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10624,11 +10624,22 @@ void Compiler::impImportBlockCode(BasicBlock* block)
SPILL_APPEND:

// We need to call impSpillLclRefs() for a struct type lclVar.
// This is done for non-block assignments in the handling of stloc.
if ((op1->OperGet() == GT_ASG) && varTypeIsStruct(op1->gtOp.gtOp1) &&
(op1->gtOp.gtOp1->gtOper == GT_LCL_VAR))
if ((op1->OperGet() == GT_ASG) && varTypeIsStruct(op1->gtGetOp1()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to also describe why we need to spill in the first place.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do.

{
impSpillLclRefs(op1->gtOp.gtOp1->AsLclVarCommon()->gtLclNum);
GenTree* lhs = op1->gtGetOp1();
GenTreeLclVarCommon* lclVar = nullptr;
if (lhs->gtOper == GT_LCL_VAR)
{
lclVar = lhs->AsLclVarCommon();
}
else if (lhs->OperIsBlk())
{
lclVar = lhs->AsBlk()->Addr()->IsLocalAddrExpr();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is IsLocalAddrExpr really the right function to use here? There are a few that attempt to detect local addresses and they differ in the trees that they recognize. This one recognizes something like ADD(ADDR(LCL_VAR, 2) but that's less common during import. There's also impIsAddressInLocal that does something similar but instead of ADD nodes it looks for FIELD nodes, which of course are more common during import.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you're right. IsLocalAddrExpr() would be the right thing to call after morph, but at this point we might see a FIELD. I'm going to try to get a repro case for that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having spent some time on this, I'm fairly certain that this only needs to handle full assignments. The FIELD case is handled here: https://github.com/dotnet/coreclr/blob/master/src/jit/importer.cpp#L14507, and explicit stloc cases here: https://github.com/dotnet/coreclr/blob/master/src/jit/importer.cpp#L11103
.In the failure case the "assignment" is an initobj. The block cases handle this with a goto SPILL_APPEND which lands here. Pretty much anything else that would modify a local would involve explicitly taking its address (not just in the context of doing a field store).

I gave some thought to whether an assert could be added, but I was unable to do so. So, I'm actually going to simplify this check.

}
if (lclVar != nullptr)
{
impSpillLclRefs(lclVar->gtLclNum);
}
}

/* Append 'op1' to the list of statements */
Expand Down
3 changes: 0 additions & 3 deletions tests/issues.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,6 @@
<ExcludeList Include="$(XunitTestBinBase)/tracing/keyword/TwoKeywords/TwoKeywords/*">
<Issue>23224, often fails with timeout in release</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/JIT/Methodical/flowgraph/dev10_bug679955/volatileLocal1/*">
<Issue>23545</Issue>
</ExcludeList>
</ItemGroup>

<!-- All Unix targets -->
Expand Down
53 changes: 53 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_23545/GitHub_23545.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
// 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.Collections.Generic;

namespace GitHub_23545
{
using System;
using System.Collections.Generic;

public struct TestStruct
{
public int value1;

public override string ToString()
{
return this.value1.ToString();
}
}

class Test
{
public static Dictionary<TestStruct, TestStruct> StructKeyValue
{
get
{
return new Dictionary<TestStruct, TestStruct>()
{
{
new TestStruct(){value1 = 12}, new TestStruct(){value1 = 15}
}
};
}
}

static int Main()
{
int value = 0;
foreach (var e in StructKeyValue)
{
value += e.Key.value1 + e.Value.value1;
Console.WriteLine(e.Key.ToString() + " " + e.Value.ToString());
}
if (value != 27)
{
return -1;
}
return 100;
}
}
}
17 changes: 17 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_23545/GitHub_23545.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<?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)' == '' ">Release</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<AssemblyName>$(MSBuildProjectName)</AssemblyName>
<OutputType>Exe</OutputType>
<DebugType></DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
</Project>