From 575c4c88e8f5cd4c821f32d36ed276cc1c466294 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 4 Feb 2022 11:53:55 +0100 Subject: [PATCH] Track local var ranges for "interfering writes" LIR check 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 #57919 which made the fuzzer jobs very noisy and made it easy to miss actual new examples (e.g. #63720 was just merged even though there were real examples found there). Fix #57919 --- src/coreclr/jit/lir.cpp | 69 ++++++++++++++++++++++++++------- src/coreclr/jit/sideeffects.cpp | 11 ++++-- src/coreclr/jit/sideeffects.h | 7 ++++ 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/lir.cpp b/src/coreclr/jit/lir.cpp index 81d03b93f7b347..2f5b762214ef17 100644 --- a/src/coreclr/jit/lir.cpp +++ b/src/coreclr/jit/lir.cpp @@ -1338,7 +1338,10 @@ class CheckLclVarSemanticsHelper CheckLclVarSemanticsHelper(Compiler* compiler, const LIR::Range* range, SmallHashTable& unusedDefs) - : compiler(compiler), range(range), unusedDefs(unusedDefs), unusedLclVarReads(compiler->getAllocator()) + : compiler(compiler) + , range(range) + , unusedDefs(unusedDefs) + , unusedLclVarReads(compiler->getAllocator(CMK_DebugOnly)) { } @@ -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* reads; + if (!unusedLclVarReads.TryGetValue(nodeInfo.LclNum(), &reads)) + { + reads = new (compiler, CMK_DebugOnly) jitstd::list(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* 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(*range), read); + JITDUMP("Write:\n"); + DISPTREERANGE(const_cast(*range), node); + assert(!"Write to unaliased local overlaps outstanding read"); + break; + } + } + } + } } return true; @@ -1393,14 +1428,22 @@ class CheckLclVarSemanticsHelper AliasSet::NodeInfo operandInfo(compiler, operand); if (operandInfo.IsLclVarRead()) { - int count; - const bool removed = unusedLclVarReads.TryRemove(operandInfo.LclNum(), &count); - assert(removed); + jitstd::list* reads; + const bool foundList = unusedLclVarReads.TryGetValue(operandInfo.LclNum(), &reads); + assert(foundList); - if (count > 1) + bool found = false; + for (jitstd::list::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"); } } } @@ -1408,8 +1451,8 @@ class CheckLclVarSemanticsHelper private: Compiler* compiler; const LIR::Range* range; - SmallHashTable& unusedDefs; - SmallHashTable unusedLclVarReads; + SmallHashTable& unusedDefs; + SmallHashTable*, 16U> unusedLclVarReads; }; //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/sideeffects.cpp b/src/coreclr/jit/sideeffects.cpp index b7c896d14098fd..fe6a8a7d6064c5 100644 --- a/src/coreclr/jit/sideeffects.cpp +++ b/src/coreclr/jit/sideeffects.cpp @@ -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()) { @@ -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 @@ -183,6 +184,7 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node) { isLclVarAccess = true; lclNum = address->AsLclVarCommon()->GetLclNum(); + lclOffs = address->AsLclVarCommon()->GetLclOffs(); } else { @@ -197,6 +199,7 @@ AliasSet::NodeInfo::NodeInfo(Compiler* compiler, GenTree* node) { isLclVarAccess = true; lclNum = node->AsLclVarCommon()->GetLclNum(); + lclOffs = node->AsLclVarCommon()->GetLclOffs(); } else { @@ -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 @@ -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; } } } diff --git a/src/coreclr/jit/sideeffects.h b/src/coreclr/jit/sideeffects.h index 8e2fe1cd788b4b..e7ffdb0a7311b8 100644 --- a/src/coreclr/jit/sideeffects.h +++ b/src/coreclr/jit/sideeffects.h @@ -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); @@ -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;