Skip to content

Commit

Permalink
Don't morph volatile IND(ADDR(LCL_VAR)) (dotnet/coreclr#20843)
Browse files Browse the repository at this point in the history 
Besides the fact that volatile indirections aren't supposed to be removed doing so in this case results in incorrect code being generated.

The GT_IND node has GTF_DONT_CSE set and this gets copied to the GT_LCL_VAR node. Later this prevents the insertion of a normalization cast because GTF_DONT_CSE on a GT_LCL_VAR node is assumed to mean that the variable address is being taken.

Commit migrated from dotnet/coreclr@3e21684
  • Loading branch information
@mikedn
mikedn authored and Sergey Andreenko committed Dec 21, 2018
1 parent 853967f commit bf3f21f
Show file tree
Hide file tree
Showing 3 changed files with 182 additions and 125 deletions.
260 changes: 135 additions & 125 deletions src/coreclr/src/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13088,166 +13088,170 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
temp = nullptr;
ival1 = 0;

/* Try to Fold *(&X) into X */
if (op1->gtOper == GT_ADDR)
// Don't remove a volatile GT_IND, even if the address points to a local variable.
if ((tree->gtFlags & GTF_IND_VOLATILE) == 0)
{
// Can not remove a GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1))
/* Try to Fold *(&X) into X */
if (op1->gtOper == GT_ADDR)
{
break;
}

temp = op1->gtOp.gtOp1; // X
// Can not remove a GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1))
{
break;
}

// In the test below, if they're both TYP_STRUCT, this of course does *not* mean that
// they are the *same* struct type. In fact, they almost certainly aren't. If the
// address has an associated field sequence, that identifies this case; go through
// the "lcl_fld" path rather than this one.
FieldSeqNode* addrFieldSeq = nullptr; // This is an unused out parameter below.
if (typ == temp->TypeGet() && !GetZeroOffsetFieldMap()->Lookup(op1, &addrFieldSeq))
{
foldAndReturnTemp = true;
}
else if (temp->OperIsLocal())
{
unsigned lclNum = temp->gtLclVarCommon.gtLclNum;
LclVarDsc* varDsc = &lvaTable[lclNum];
temp = op1->gtOp.gtOp1; // X

// We will try to optimize when we have a promoted struct promoted with a zero lvFldOffset
if (varDsc->lvPromoted && (varDsc->lvFldOffset == 0))
// In the test below, if they're both TYP_STRUCT, this of course does *not* mean that
// they are the *same* struct type. In fact, they almost certainly aren't. If the
// address has an associated field sequence, that identifies this case; go through
// the "lcl_fld" path rather than this one.
FieldSeqNode* addrFieldSeq = nullptr; // This is an unused out parameter below.
if (typ == temp->TypeGet() && !GetZeroOffsetFieldMap()->Lookup(op1, &addrFieldSeq))
{
noway_assert(varTypeIsStruct(varDsc));
foldAndReturnTemp = true;
}
else if (temp->OperIsLocal())
{
unsigned lclNum = temp->gtLclVarCommon.gtLclNum;
LclVarDsc* varDsc = &lvaTable[lclNum];

// We will try to optimize when we have a single field struct that is being struct promoted
if (varDsc->lvFieldCnt == 1)
// We will try to optimize when we have a promoted struct promoted with a zero lvFldOffset
if (varDsc->lvPromoted && (varDsc->lvFldOffset == 0))
{
unsigned lclNumFld = varDsc->lvFieldLclStart;
// just grab the promoted field
LclVarDsc* fieldVarDsc = &lvaTable[lclNumFld];
noway_assert(varTypeIsStruct(varDsc));

// Also make sure that the tree type matches the fieldVarType and that it's lvFldOffset
// is zero
if (fieldVarDsc->TypeGet() == typ && (fieldVarDsc->lvFldOffset == 0))
// We will try to optimize when we have a single field struct that is being struct promoted
if (varDsc->lvFieldCnt == 1)
{
// We can just use the existing promoted field LclNum
temp->gtLclVarCommon.SetLclNum(lclNumFld);
temp->gtType = fieldVarDsc->TypeGet();
unsigned lclNumFld = varDsc->lvFieldLclStart;
// just grab the promoted field
LclVarDsc* fieldVarDsc = &lvaTable[lclNumFld];

foldAndReturnTemp = true;
// Also make sure that the tree type matches the fieldVarType and that it's lvFldOffset
// is zero
if (fieldVarDsc->TypeGet() == typ && (fieldVarDsc->lvFldOffset == 0))
{
// We can just use the existing promoted field LclNum
temp->gtLclVarCommon.SetLclNum(lclNumFld);
temp->gtType = fieldVarDsc->TypeGet();

foldAndReturnTemp = true;
}
}
}
}
// If the type of the IND (typ) is a "small int", and the type of the local has the
// same width, then we can reduce to just the local variable -- it will be
// correctly normalized, and signed/unsigned differences won't matter.
//
// The below transformation cannot be applied if the local var needs to be normalized on load.
else if (varTypeIsSmall(typ) && (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) &&
!lvaTable[lclNum].lvNormalizeOnLoad())
{
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}
else if (!varTypeIsStruct(typ) && (lvaTable[lclNum].lvType == typ) &&
!lvaTable[lclNum].lvNormalizeOnLoad())
{
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}
else
{
// Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e.
// nullptr)
assert(fieldSeq == nullptr);
bool b = GetZeroOffsetFieldMap()->Lookup(op1, &fieldSeq);
assert(b || fieldSeq == nullptr);

if ((fieldSeq != nullptr) && (temp->OperGet() == GT_LCL_FLD))
// If the type of the IND (typ) is a "small int", and the type of the local has the
// same width, then we can reduce to just the local variable -- it will be
// correctly normalized, and signed/unsigned differences won't matter.
//
// The below transformation cannot be applied if the local var needs to be normalized on load.
else if (varTypeIsSmall(typ) && (genTypeSize(lvaTable[lclNum].lvType) == genTypeSize(typ)) &&
!lvaTable[lclNum].lvNormalizeOnLoad())
{
// Append the field sequence, change the type.
temp->AsLclFld()->gtFieldSeq =
GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq);
temp->gtType = typ;

foldAndReturnTemp = true;
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}
}
// Otherwise will will fold this into a GT_LCL_FLD below
// where we check (temp != nullptr)
}
else // !temp->OperIsLocal()
{
// We don't try to fold away the GT_IND/GT_ADDR for this case
temp = nullptr;
}
}
else if (op1->OperGet() == GT_ADD)
{
/* Try to change *(&lcl + cns) into lcl[cns] to prevent materialization of &lcl */
else if (!varTypeIsStruct(typ) && (lvaTable[lclNum].lvType == typ) &&
!lvaTable[lclNum].lvNormalizeOnLoad())
{
tree->gtType = typ = temp->TypeGet();
foldAndReturnTemp = true;
}
else
{
// Assumes that when Lookup returns "false" it will leave "fieldSeq" unmodified (i.e.
// nullptr)
assert(fieldSeq == nullptr);
bool b = GetZeroOffsetFieldMap()->Lookup(op1, &fieldSeq);
assert(b || fieldSeq == nullptr);

if (op1->gtOp.gtOp1->OperGet() == GT_ADDR && op1->gtOp.gtOp2->OperGet() == GT_CNS_INT &&
(!(opts.MinOpts() || opts.compDbgCode)))
{
// No overflow arithmetic with pointers
noway_assert(!op1->gtOverflow());
if ((fieldSeq != nullptr) && (temp->OperGet() == GT_LCL_FLD))
{
// Append the field sequence, change the type.
temp->AsLclFld()->gtFieldSeq =
GetFieldSeqStore()->Append(temp->AsLclFld()->gtFieldSeq, fieldSeq);
temp->gtType = typ;

temp = op1->gtOp.gtOp1->gtOp.gtOp1;
if (!temp->OperIsLocal())
{
temp = nullptr;
break;
foldAndReturnTemp = true;
}
}
// Otherwise will will fold this into a GT_LCL_FLD below
// where we check (temp != nullptr)
}

// Can not remove the GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1->gtOp.gtOp1))
else // !temp->OperIsLocal()
{
break;
// We don't try to fold away the GT_IND/GT_ADDR for this case
temp = nullptr;
}
}
else if (op1->OperGet() == GT_ADD)
{
/* Try to change *(&lcl + cns) into lcl[cns] to prevent materialization of &lcl */

ival1 = op1->gtOp.gtOp2->gtIntCon.gtIconVal;
fieldSeq = op1->gtOp.gtOp2->gtIntCon.gtFieldSeq;

// Does the address have an associated zero-offset field sequence?
FieldSeqNode* addrFieldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(op1->gtOp.gtOp1, &addrFieldSeq))
if (op1->gtOp.gtOp1->OperGet() == GT_ADDR && op1->gtOp.gtOp2->OperGet() == GT_CNS_INT &&
(!(opts.MinOpts() || opts.compDbgCode)))
{
fieldSeq = GetFieldSeqStore()->Append(addrFieldSeq, fieldSeq);
}
// No overflow arithmetic with pointers
noway_assert(!op1->gtOverflow());

if (ival1 == 0 && typ == temp->TypeGet() && temp->TypeGet() != TYP_STRUCT)
{
noway_assert(!varTypeIsGC(temp->TypeGet()));
foldAndReturnTemp = true;
}
else
{
// The emitter can't handle large offsets
if (ival1 != (unsigned short)ival1)
temp = op1->gtOp.gtOp1->gtOp.gtOp1;
if (!temp->OperIsLocal())
{
temp = nullptr;
break;
}

// The emitter can get confused by invalid offsets
if (ival1 >= Compiler::lvaLclSize(temp->gtLclVarCommon.gtLclNum))
// Can not remove the GT_ADDR if it is currently a CSE candidate.
if (gtIsActiveCSE_Candidate(op1->gtOp.gtOp1))
{
break;
}

#ifdef _TARGET_ARM_
// Check for a LclVar TYP_STRUCT with misalignment on a Floating Point field
//
if (varTypeIsFloating(typ))
ival1 = op1->gtOp.gtOp2->gtIntCon.gtIconVal;
fieldSeq = op1->gtOp.gtOp2->gtIntCon.gtFieldSeq;

// Does the address have an associated zero-offset field sequence?
FieldSeqNode* addrFieldSeq = nullptr;
if (GetZeroOffsetFieldMap()->Lookup(op1->gtOp.gtOp1, &addrFieldSeq))
{
fieldSeq = GetFieldSeqStore()->Append(addrFieldSeq, fieldSeq);
}

if (ival1 == 0 && typ == temp->TypeGet() && temp->TypeGet() != TYP_STRUCT)
{
noway_assert(!varTypeIsGC(temp->TypeGet()));
foldAndReturnTemp = true;
}
else
{
if ((ival1 % emitTypeSize(typ)) != 0)
// The emitter can't handle large offsets
if (ival1 != (unsigned short)ival1)
{
tree->gtFlags |= GTF_IND_UNALIGNED;
break;
}
}

// The emitter can get confused by invalid offsets
if (ival1 >= Compiler::lvaLclSize(temp->gtLclVarCommon.gtLclNum))
{
break;
}

#ifdef _TARGET_ARM_
// Check for a LclVar TYP_STRUCT with misalignment on a Floating Point field
//
if (varTypeIsFloating(typ))
{
if ((ival1 % emitTypeSize(typ)) != 0)
{
tree->gtFlags |= GTF_IND_UNALIGNED;
break;
}
}
#endif
}
// Now we can fold this into a GT_LCL_FLD below
// where we check (temp != nullptr)
}
// Now we can fold this into a GT_LCL_FLD below
// where we check (temp != nullptr)
}
}

Expand Down Expand Up @@ -18205,7 +18209,13 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
assert(TopValue(1).Node() == node);
assert(TopValue(0).Node() == node->gtGetOp1());

if (!TopValue(1).Indir(TopValue(0)))
if ((node->gtFlags & GTF_IND_VOLATILE) != 0)
{
// Volatile indirections must not be removed so the address,
// if any, must be escaped.
EscapeValue(TopValue(0), node);
}
else if (!TopValue(1).Indir(TopValue(0)))
{
// If the address comes from another indirection (e.g. IND(IND(...))
// then we need to escape the location.
Expand Down
30 changes: 30 additions & 0 deletions src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// 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.Runtime.CompilerServices;
using System.Threading;

struct S0
{
public byte F0;
}

class Program
{
static S0 s_2;
static long s_5;

static int Main()
{
s_2.F0 = 128;
M7(s_2);
return (s_5 == 128) ? 100 : -1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static void M7(S0 arg0)
{
s_5 = Volatile.Read(ref arg0.F0);
}
}
17 changes: 17 additions & 0 deletions src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_19599/GitHub_19599.csproj
View file
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>

0 comments on commit bf3f21f

Please sign in to comment.