Skip to content

Commit

Permalink
Track local var ranges for "interfering writes" LIR check
Browse files Browse the repository at this point in the history 
For LIR we verify that we can really consider locals to be used at their
user by having a checker that looks for interfering stores to the same
locals. However, in some cases we may have "interfering"
GT_LCL_FLD/GT_STORE_LCL_FLD, in the sense that they work on the same
local but on a disjoint range of bytes. Add support to validate this.

This fixes dotnet#57919 which made the fuzzer jobs very noisy and made it easy
to miss actual new examples (e.g. dotnet#63720 was just merged even though
there were real examples found there).

Fix dotnet#57919
  • Loading branch information
@jakobbotsch
jakobbotsch committed Feb 4, 2022
1 parent 607dcce commit 575c4c8
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 16 deletions.
69 changes: 56 additions & 13 deletions src/coreclr/jit/lir.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1338,7 +1338,10 @@ class CheckLclVarSemanticsHelper
CheckLclVarSemanticsHelper(Compiler* compiler,
const LIR::Range* range,
SmallHashTable<GenTree*, bool, 32U>& unusedDefs)
: compiler(compiler), range(range), unusedDefs(unusedDefs), unusedLclVarReads(compiler->getAllocator())
: compiler(compiler)
, range(range)
, unusedDefs(unusedDefs)
, unusedLclVarReads(compiler->getAllocator(CMK_DebugOnly))
{
}

Expand All @@ -1358,13 +1361,45 @@ class CheckLclVarSemanticsHelper
AliasSet::NodeInfo nodeInfo(compiler, node);
if (nodeInfo.IsLclVarRead() && !unusedDefs.Contains(node))
{
int count = 0;
unusedLclVarReads.TryGetValue(nodeInfo.LclNum(), &count);
unusedLclVarReads.AddOrUpdate(nodeInfo.LclNum(), count + 1);
jitstd::list<GenTree*>* reads;
if (!unusedLclVarReads.TryGetValue(nodeInfo.LclNum(), &reads))
{
reads = new (compiler, CMK_DebugOnly) jitstd::list<GenTree*>(compiler->getAllocator(CMK_DebugOnly));
unusedLclVarReads.AddOrUpdate(nodeInfo.LclNum(), reads);
}

reads->push_back(node);
}

// If this node is a lclVar write, it must be to a lclVar that does not have an outstanding read.
assert(!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum()));
if (nodeInfo.IsLclVarWrite())
{
// If this node is a lclVar write, it must be not alias a lclVar with an outstanding read
jitstd::list<GenTree*>* reads;
if (unusedLclVarReads.TryGetValue(nodeInfo.LclNum(), &reads))
{
for (GenTree* read : *reads)
{
AliasSet::NodeInfo readInfo(compiler, read);
assert(readInfo.IsLclVarRead() && readInfo.LclNum() == nodeInfo.LclNum());
unsigned readStart = readInfo.LclOffs();
unsigned readEnd = readStart + genTypeSize(read->TypeGet());
unsigned writeStart = nodeInfo.LclOffs();
unsigned writeEnd = writeStart + genTypeSize(node->TypeGet());
if ((readEnd > writeStart) && (writeEnd > readStart))
{
JITDUMP(
"Write to unaliased local overlaps outstanding read (write: %u..%u, read: %u..%u)\n",
writeStart, writeEnd, readStart, readEnd);
JITDUMP("Read:\n");
DISPTREERANGE(const_cast<LIR::Range&>(*range), read);
JITDUMP("Write:\n");
DISPTREERANGE(const_cast<LIR::Range&>(*range), node);
assert(!"Write to unaliased local overlaps outstanding read");
break;
}
}
}
}
}

return true;
Expand Down Expand Up @@ -1393,23 +1428,31 @@ class CheckLclVarSemanticsHelper
AliasSet::NodeInfo operandInfo(compiler, operand);
if (operandInfo.IsLclVarRead())
{
int count;
const bool removed = unusedLclVarReads.TryRemove(operandInfo.LclNum(), &count);
assert(removed);
jitstd::list<GenTree*>* reads;
const bool foundList = unusedLclVarReads.TryGetValue(operandInfo.LclNum(), &reads);
assert(foundList);

if (count > 1)
bool found = false;
for (jitstd::list<GenTree*>::iterator it = reads->begin(); it != reads->end(); ++it)
{
unusedLclVarReads.AddOrUpdate(operandInfo.LclNum(), count - 1);
if (*it == operand)
{
reads->erase(it);
found = true;
break;
}
}

assert(found || !"Could not find consumed local in unusedLclVarReads");
}
}
}

private:
Compiler* compiler;
const LIR::Range* range;
SmallHashTable<GenTree*, bool, 32U>& unusedDefs;
SmallHashTable<int, int, 32U> unusedLclVarReads;
SmallHashTable<GenTree*, bool, 32U>& unusedDefs;
SmallHashTable<int, jitstd::list<GenTree*>*, 16U> unusedLclVarReads;
};

//------------------------------------------------------------------------
Expand Down
11 changes: 8 additions & 3 deletions src/coreclr/jit/sideeffects.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ AliasSet::AliasSet()
// node - The node in question.
//
AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
: m_compiler(compiler), m_node(node), m_flags(0), m_lclNum(0)
: m_compiler(compiler), m_node(node), m_flags(0), m_lclNum(0), m_lclOffs(0)
{
if (node->IsCall())
{
Expand Down Expand Up @@ -174,6 +174,7 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
bool isMemoryAccess = false;
bool isLclVarAccess = false;
unsigned lclNum = 0;
unsigned lclOffs = 0;
if (node->OperIsIndir())
{
// If the indirection targets a lclVar, we can be more precise with regards to aliasing by treating the
Expand All @@ -183,6 +184,7 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
{
isLclVarAccess = true;
lclNum = address->AsLclVarCommon()->GetLclNum();
lclOffs = address->AsLclVarCommon()->GetLclOffs();
}
else
{
Expand All @@ -197,6 +199,7 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
{
isLclVarAccess = true;
lclNum = node->AsLclVarCommon()->GetLclNum();
lclOffs = node->AsLclVarCommon()->GetLclOffs();
}
else
{
Expand All @@ -221,7 +224,8 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
if (isLclVarAccess)
{
m_flags |= ALIAS_READS_LCL_VAR;
m_lclNum = lclNum;
m_lclNum = lclNum;
m_lclOffs = lclOffs;
}
}
else
Expand All @@ -234,7 +238,8 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node)
if (isLclVarAccess)
{
m_flags |= ALIAS_WRITES_LCL_VAR;
m_lclNum = lclNum;
m_lclNum = lclNum;
m_lclOffs = lclOffs;
}
}
}
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/jit/sideeffects.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class AliasSet final
GenTree* m_node;
unsigned m_flags;
unsigned m_lclNum;
unsigned m_lclOffs;

public:
NodeInfo(Compiler* compiler, GenTree* node);
Expand Down Expand Up @@ -112,6 +113,12 @@ class AliasSet final
return m_lclNum;
}

inline unsigned LclOffs() const
{
assert(IsLclVarRead() || IsLclVarWrite());
return m_lclOffs;
}

inline bool WritesAnyLocation() const
{
return (m_flags & (ALIAS_WRITES_ADDRESSABLE_LOCATION | ALIAS_WRITES_LCL_VAR)) != 0;
Expand Down

0 comments on commit 575c4c8

Please sign in to comment.