Permalink
Browse files

Fix evaluation order for block copy

This order was changed five years ago, as a workaround for data flow analysis issues around the block copy. Now that block copies are handled as regular assignments, this is no longer needed - and is, in fact, incorrect as it causes side effects to be handled in the wrong order.

Fix dotnet#19243
  • Loading branch information...
CarolEidt committed Aug 7, 2018
1 parent d1c6a93 commit f35e6085602a7f11437a0e90e940ba418f69bac4
View
@@ -18526,11 +18526,13 @@ void Compiler::fgSetTreeSeqHelper(GenTree* tree, bool isLIR)
fgSetTreeSeqHelper(dstAddr, isLIR);
fgSetTreeSeqHelper(src, isLIR);
}
// If the size is not evaluated first, it is treated as a child of the assignment,
// so we finish its evaluation *after* the dynBlk.
fgSetTreeSeqFinish(dynBlk, isLIR);
if (!dynBlk->gtEvalSizeFirst)
{
fgSetTreeSeqHelper(sizeNode, isLIR);
}
fgSetTreeSeqFinish(dynBlk, isLIR);
if (asg != nullptr)
{
fgSetTreeSeqFinish(asg, isLIR);
@@ -21535,6 +21537,20 @@ void Compiler::fgDebugCheckNodeLinks(BasicBlock* block, GenTree* node)
expectedPrevTree = tree->AsColon()->ElseNode(); // "else" branch result (generated first).
break;
case GT_ASG:
// Special case for assignment of dynamic block.
// This is here to duplicate the former case where the size may be evaluated prior to the
// source and destination addresses. In order to do this, we treat the size as a child of the
// assignment.
// TODO-1stClassStructs-Cleanup: Remove all this special casing, and ensure that the diffs are
// reasonable.
if ((tree->gtGetOp1()->OperGet() == GT_DYN_BLK) && (!tree->gtGetOp1()->AsDynBlk()->gtEvalSizeFirst))
{
expectedPrevTree = tree->gtGetOp1()->AsDynBlk()->gtDynamicSize;
break;
}
__fallthrough;
default:
if (tree->gtOp.gtOp2)
{
View
@@ -4203,17 +4203,16 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
switch (op1Val->gtOper)
{
case GT_IND:
case GT_BLK:
case GT_OBJ:
case GT_DYN_BLK:
// Struct assignments are different from scalar assignments in that semantically
// the address of op1 is evaluated prior to op2.
if (!varTypeIsStruct(op1))
// In an indirection, the destination address is evaluated prior to the source.
// If we have any side effects on the target indirection,
// we have to evaluate op1 first.
if (op1Val->AsIndir()->Addr()->gtFlags & GTF_ALL_EFFECT)
{
// If we have any side effects on the GT_IND child node
// we have to evaluate op1 first.
if (op1Val->gtOp.gtOp1->gtFlags & GTF_ALL_EFFECT)
{
break;
}
break;
}
// In case op2 assigns to a local var that is used in op1Val, we have to evaluate op1Val first.
@@ -4233,9 +4232,6 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
case GT_LCL_VAR:
case GT_LCL_FLD:
case GT_BLK:
case GT_OBJ:
case GT_DYN_BLK:
// We evaluate op2 before op1
bReverseInAssignment = true;
@@ -6912,9 +6908,6 @@ void Compiler::gtBlockOpInit(GenTree* result, GenTree* dst, GenTree* srcOrFillVa
result->gtFlags |= dst->gtFlags & GTF_ALL_EFFECT;
result->gtFlags |= result->gtOp.gtOp2->gtFlags & GTF_ALL_EFFECT;
// REVERSE_OPS is necessary because the use must occur before the def
result->gtFlags |= GTF_REVERSE_OPS;
result->gtFlags |= (dst->gtFlags & GTF_EXCEPT) | (srcOrFillVal->gtFlags & GTF_EXCEPT);
if (isVolatile)
View
@@ -551,7 +551,10 @@ void Rationalizer::RewriteAssignment(LIR::Use& use)
(assignment->gtFlags & (GTF_ALL_EFFECT | GTF_BLK_VOLATILE | GTF_BLK_UNALIGNED | GTF_DONT_CSE));
storeBlk->gtBlk.Data() = value;
// Replace the assignment node with the store
// Remove the block node from its current position and replace the assignment node with it
// (now in its store form).
BlockRange().Remove(storeBlk);
BlockRange().InsertBefore(assignment, storeBlk);
use.ReplaceWith(comp, storeBlk);
BlockRange().Remove(assignment);
DISPTREERANGE(BlockRange(), use.Def());
@@ -0,0 +1,46 @@
// 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.
//
// This test exposed a bug with the ordering of evaluation of a cpblk.
struct S0
{
public long F0;
public sbyte F4;
public S0(long f0): this() { F0 = f0; }
}
class C0
{
public S0 F5;
public C0(S0 f5) { F5 = f5; }
}
public class GitHub_19243
{
static C0 s_13 = new C0(new S0(0));
static S0 s_37;
public static int checkValue(long value)
{
if (value != 8614979244451975600L)
{
System.Console.WriteLine("s_37.F0 was " + value + "; expected 8614979244451975600L");
return -1;
}
return 100;
}
static ref S0 M7()
{
s_13 = new C0(new S0(8614979244451975600L));
return ref s_37;
}
public static int Main()
{
M7() = s_13.F5;
return checkValue(s_37.F0);
}
}
@@ -0,0 +1,35 @@
<?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>{ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C}</ProjectGuid>
<OutputType>Exe</OutputType>
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
<CLRTestPriority>1</CLRTestPriority>
</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>None</DebugType>
<Optimize>False</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="GitHub_19243.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>
@@ -0,0 +1,35 @@
<?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>{ADEEA3D1-B67B-456E-8F2B-6DCCACC2D34C}</ProjectGuid>
<OutputType>Exe</OutputType>
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
<CLRTestPriority>1</CLRTestPriority>
</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>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="GitHub_19243.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>

0 comments on commit f35e608

Please sign in to comment.