Skip to content

Commit

Permalink
Update full-struct references to promoted IBR args
Browse files Browse the repository at this point in the history 
Update fgMorphImplicitByRefArgs to rewrite references to struct-promoted
implicit-by-reference parameters as references to the new struct temps
created in fgRetypeImplicitByRefArgs; fgMorphStructField isn't updating
these because it's only interested in field references, and runs upstream
of fgRetypeImplicitByRefArgs where the full struct temp is created,
anyway.
Invert the sense of lvPromoted for implicit byref args in the interim
between fgRetypeImplicitByRefArgs and fgMarkDemotedImplicitByRefArgs,
since now fgMarkDemotedImplicitByRefArgs needs to update both and would
look horribly backwards the other way around.

Fixes #11814.
  • Loading branch information
@JosephTremoulet
JosephTremoulet committed May 24, 2017
1 parent b2164d2 commit 0057a2f
Show file tree
Hide file tree
Showing 4 changed files with 174 additions and 53 deletions.
3 changes: 1 addition & 2 deletions src/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4879,8 +4879,7 @@ class Compiler
bool fgMorphImplicitByRefArgs(GenTreePtr tree);
GenTreePtr fgMorphImplicitByRefArgs(GenTreePtr tree, bool isAddr);

// Clear up annotations for any struct promotion temps created for implicit byrefs that
// wound up unused (due e.g. to being address-exposed and not worth promoting).
// Clear up annotations for any struct promotion temps created for implicit byrefs.
void fgMarkDemotedImplicitByRefArgs();

static fgWalkPreFn fgMarkAddrTakenLocalsPreCB;
Expand Down
136 changes: 85 additions & 51 deletions src/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17896,19 +17896,32 @@ void Compiler::fgRetypeImplicitByRefArgs()
fieldVarDsc->lvPrefReg = 0;
}

if (undoPromotion)
{
// Hijack lvFieldLclStart to record the new temp number.
// It will get fixed up in fgMarkDemotedImplicitByRefArgs.
varDsc->lvFieldLclStart = newLclNum;
}
else
{
// Unmark the parameter as promoted (it's a pointer now).
varDsc->lvPromoted = false;
varDsc->lvFieldCnt = 0;
varDsc->lvFieldLclStart = 0;
}
// Hijack lvFieldLclStart to record the new temp number.
// It will get fixed up in fgMarkDemotedImplicitByRefArgs.
varDsc->lvFieldLclStart = newLclNum;
// Go ahead and clear lvFieldCnt -- either we're promoting
// a replacement temp or we're not promoting this arg, and
// in either case the parameter is now a pointer that doesn't
// have these fields.
varDsc->lvFieldCnt = 0;

// Hijack lvPromoted to communicate to fgMorphImplicitByRefArgs
// whether references to the struct should be rewritten as
// indirections off the pointer (not promoted) or references
// to the new struct local (promoted).
varDsc->lvPromoted = !undoPromotion;
}
else
{
// The "undo promotion" path above clears lvPromoted for args that struct
// promotion wanted to promote but that aren't considered profitable to
// rewrite. It hijacks lvFieldLclStart to communicate to
// fgMarkDemotedImplicitByRefArgs that it needs to clean up annotations left
// on such args for fgMorphImplicitByRefArgs to consult in the interim.
// Here we have an arg that was simply never promoted, so make sure it doesn't
// have nonzero lvFieldLclStart, since that would confuse fgMorphImplicitByRefArgs
// and fgMarkDemotedImplicitByRefArgs.
assert(varDsc->lvFieldLclStart == 0);
}

// Since the parameter in this position is really a pointer, its type is TYP_BYREF.
Expand Down Expand Up @@ -17945,10 +17958,9 @@ void Compiler::fgRetypeImplicitByRefArgs()

//------------------------------------------------------------------------
// fgMarkDemotedImplicitByRefArgs: Clear annotations for any implicit byrefs that struct promotion
// asked to promote but for which fgRetypeImplicitByRefArgs decided
// to discard the promotion. Appearances of these have now been
// rewritten (by fgMorphImplicitByRefArgs) using indirections from
// the pointer parameter.
// asked to promote. Appearances of these have now been rewritten
// (by fgMorphImplicitByRefArgs) using indirections from the pointer
// parameter or references to the promotion temp, as appropriate.

void Compiler::fgMarkDemotedImplicitByRefArgs()
{
Expand All @@ -17958,45 +17970,58 @@ void Compiler::fgMarkDemotedImplicitByRefArgs()
{
LclVarDsc* varDsc = &lvaTable[lclNum];

if (lvaIsImplicitByRefLocal(lclNum) && varDsc->lvPromoted)
{
// We stashed the pointer to the real promotion temp in lvFieldLclStart
unsigned structLclNum = varDsc->lvFieldLclStart;

// Unmark the parameter as promoted.
varDsc->lvPromoted = false;
varDsc->lvFieldCnt = 0;
varDsc->lvFieldLclStart = 0;
// Clear its ref count; this was set during address-taken analysis so that
// call morphing could identify single-use implicit byrefs; we're done with
// that, and want it to be in its default state of zero when we go to set
// real ref counts for all variables.
varDsc->lvRefCnt = 0;

// The temp struct is now unused; set flags appropriately so that we
// won't allocate space for it on the stack.
LclVarDsc* structVarDsc = &lvaTable[structLclNum];
structVarDsc->lvRefCnt = 0;
structVarDsc->lvAddrExposed = false;
if (lvaIsImplicitByRefLocal(lclNum))
{
if (varDsc->lvPromoted)
{
// The parameter is simply a pointer now, so clear lvPromoted.
varDsc->lvPromoted = false;

// Clear the lvFieldLclStart value that was set by fgRetypeImplicitByRefArgs
// to tell fgMorphImplicitByRefArgs how to rewrite appearances of this arg.
varDsc->lvFieldLclStart = 0;
}
else if (varDsc->lvFieldLclStart != 0)
{
// We created new temps to represent a promoted struct corresponding to this
// parameter, but decided not to go through with the promotion and have
// rewritten all uses as indirections off the pointer parameter.
// We stashed the pointer to the new struct temp in lvFieldLclStart; make
// note of that and clear the annotation.
unsigned structLclNum = varDsc->lvFieldLclStart;
varDsc->lvFieldLclStart = 0;

// Clear the arg's ref count; this was set during address-taken analysis so that
// call morphing could identify single-use implicit byrefs; we're done with
// that, and want it to be in its default state of zero when we go to set
// real ref counts for all variables.
varDsc->lvRefCnt = 0;

// The temp struct is now unused; set flags appropriately so that we
// won't allocate space for it on the stack.
LclVarDsc* structVarDsc = &lvaTable[structLclNum];
structVarDsc->lvRefCnt = 0;
structVarDsc->lvAddrExposed = false;
#ifdef DEBUG
structVarDsc->lvUnusedStruct = true;
structVarDsc->lvUnusedStruct = true;
#endif // DEBUG

unsigned fieldLclStart = structVarDsc->lvFieldLclStart;
unsigned fieldCount = structVarDsc->lvFieldCnt;
unsigned fieldLclStop = fieldLclStart + fieldCount;
unsigned fieldLclStart = structVarDsc->lvFieldLclStart;
unsigned fieldCount = structVarDsc->lvFieldCnt;
unsigned fieldLclStop = fieldLclStart + fieldCount;

for (unsigned fieldLclNum = fieldLclStart; fieldLclNum < fieldLclStop; ++fieldLclNum)
{
// Fix the pointer to the parent local.
LclVarDsc* fieldVarDsc = &lvaTable[fieldLclNum];
assert(fieldVarDsc->lvParentLcl == lclNum);
fieldVarDsc->lvParentLcl = structLclNum;
for (unsigned fieldLclNum = fieldLclStart; fieldLclNum < fieldLclStop; ++fieldLclNum)
{
// Fix the pointer to the parent local.
LclVarDsc* fieldVarDsc = &lvaTable[fieldLclNum];
assert(fieldVarDsc->lvParentLcl == lclNum);
fieldVarDsc->lvParentLcl = structLclNum;

// The field local is now unused; set flags appropriately so that
// we won't allocate stack space for it.
fieldVarDsc->lvRefCnt = 0;
fieldVarDsc->lvAddrExposed = false;
// The field local is now unused; set flags appropriately so that
// we won't allocate stack space for it.
fieldVarDsc->lvRefCnt = 0;
fieldVarDsc->lvAddrExposed = false;
}
}
}
}
Expand Down Expand Up @@ -18074,8 +18099,17 @@ GenTreePtr Compiler::fgMorphImplicitByRefArgs(GenTreePtr tree, bool isAddr)
if (!varTypeIsStruct(lclVarTree))
{
assert(lclVarTree->TypeGet() == TYP_BYREF);

return nullptr;
}
else if (lclVarDsc->lvPromoted)
{
// fgRetypeImplicitByRefArgs created a new promoted struct local to represent this
// arg. Rewrite this to refer to the new local.
assert(lclVarDsc->lvFieldLclStart != 0);
lclVarTree->AsLclVarCommon()->SetLclNum(lclVarDsc->lvFieldLclStart);
return tree;
}

fieldHnd = nullptr;
}
Expand Down
45 changes: 45 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// 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.

// Repro case for a bug involving failure to rewrite all references
// to a promoted implicit byref struct parameter.

using System;
using System.Runtime.CompilerServices;

class MutateStructArg
{
public struct P
{
public string S;
public int X;
}

public static int Main()
{
P l1 = new P();
l1.S = "Hello World";
l1.X = 42;
P l2 = foo(l1);
Console.WriteLine(l2.S); // Print modified value "Goodbye World"
if ((l2.S == "Goodbye World") && (l2.X == 100))
{
return 100; // success
}
else
{
Console.WriteLine("**** Test FAILED ***");
return 1; // failure
}
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static P foo(P a)
{
Console.WriteLine(a.S); // Print the incoming value "Hello World"
a.S = "Goodbye World"; // Mutate the incoming value
a.X = 100;
return a; // Copy the modified value to the return value (bug was that this was returning original unmodified arg)
}
}
43 changes: 43 additions & 0 deletions tests/src/JIT/Regression/JitBlue/GitHub_11814/GitHub_11814.csproj
View file
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="GitHub_11814.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 0057a2f

Please sign in to comment.