From 6fc16186bd71d97e422f7c527dfa529c0b94bd5e Mon Sep 17 00:00:00 2001 From: Joseph Tremoulet Date: Fri, 30 Dec 2016 08:43:32 -0800 Subject: [PATCH] Simplify HeapPhiArg There's no need to have two "modes" of heap phi args; we can simply always store the SSA number, which has always already been computed and stored in the bbHeapSsaNumOut field of the pred already when the HeapPhiArg is created. We weren't using the block for anything other than to fetch the pred's ssa-out number. The code to add a try-begin block's predecessors' live-out heap defs to the try's handlers' phis used to assert that the phi args being added correspond to different blocks. While they do correspond to different blocks, it turns out that sometimes those different blocks have the same live-out heap def. This redundancy hasn't been hurting anything, and de-duplication could be fairly expensive, so in this change I've simply removed the assertion and left the redundancy as-is. --- src/jit/block.h | 24 +++--------------------- src/jit/ssabuilder.cpp | 27 +++++++++++++-------------- 2 files changed, 16 insertions(+), 35 deletions(-) diff --git a/src/jit/block.h b/src/jit/block.h index add5451..40a1c35 100644 --- a/src/jit/block.h +++ b/src/jit/block.h @@ -818,33 +818,15 @@ struct BasicBlock : private LIR::Range // lclVar, and thus has no local #, we can't use a GenTreePhiArg. Instead, we use this struct. struct HeapPhiArg { - bool m_isSsaNum; // If true, the phi arg is an SSA # for an internal try block heap state, being - // added to the phi of a catch block. If false, it's a pred block. - union { - BasicBlock* m_predBB; // Predecessor block from which the SSA # flows. - unsigned m_ssaNum; // SSA# for internal block heap state. - }; + unsigned m_ssaNum; // SSA# for incoming value. HeapPhiArg* m_nextArg; // Next arg in the list, else NULL. unsigned GetSsaNum() { - if (m_isSsaNum) - { - return m_ssaNum; - } - else - { - assert(m_predBB != nullptr); - return m_predBB->bbHeapSsaNumOut; - } + return m_ssaNum; } - HeapPhiArg(BasicBlock* predBB, HeapPhiArg* nextArg = nullptr) - : m_isSsaNum(false), m_predBB(predBB), m_nextArg(nextArg) - { - } - HeapPhiArg(unsigned ssaNum, HeapPhiArg* nextArg = nullptr) - : m_isSsaNum(true), m_ssaNum(ssaNum), m_nextArg(nextArg) + HeapPhiArg(unsigned ssaNum, HeapPhiArg* nextArg = nullptr) : m_ssaNum(ssaNum), m_nextArg(nextArg) { } diff --git a/src/jit/ssabuilder.cpp b/src/jit/ssabuilder.cpp index f0ee461..5d553c1 100644 --- a/src/jit/ssabuilder.cpp +++ b/src/jit/ssabuilder.cpp @@ -1333,17 +1333,18 @@ void SsaBuilder::AssignPhiNodeRhsVariables(BasicBlock* block, SsaRenameState* pR { if (succ->bbHeapSsaPhiFunc == BasicBlock::EmptyHeapPhiDef) { - succ->bbHeapSsaPhiFunc = new (m_pCompiler) BasicBlock::HeapPhiArg(block); + succ->bbHeapSsaPhiFunc = new (m_pCompiler) BasicBlock::HeapPhiArg(block->bbHeapSsaNumOut); } else { BasicBlock::HeapPhiArg* curArg = succ->bbHeapSsaPhiFunc; + unsigned ssaNum = block->bbHeapSsaNumOut; bool found = false; // This is a quadratic algorithm. We might need to consider some switch over to a hash table // representation for the arguments of a phi node, to make this linear. while (curArg != nullptr) { - if (curArg->m_predBB == block) + if (curArg->m_ssaNum == ssaNum) { found = true; break; @@ -1352,7 +1353,7 @@ void SsaBuilder::AssignPhiNodeRhsVariables(BasicBlock* block, SsaRenameState* pR } if (!found) { - succ->bbHeapSsaPhiFunc = new (m_pCompiler) BasicBlock::HeapPhiArg(block, succ->bbHeapSsaPhiFunc); + succ->bbHeapSsaPhiFunc = new (m_pCompiler) BasicBlock::HeapPhiArg(ssaNum, succ->bbHeapSsaPhiFunc); } } DBG_SSA_JITDUMP(" Added phi arg for Heap from BB%02u in BB%02u.\n", block->bbNum, succ->bbNum); @@ -1466,20 +1467,18 @@ void SsaBuilder::AssignPhiNodeRhsVariables(BasicBlock* block, SsaRenameState* pR { if (handlerStart->bbHeapSsaPhiFunc == BasicBlock::EmptyHeapPhiDef) { - handlerStart->bbHeapSsaPhiFunc = new (m_pCompiler) BasicBlock::HeapPhiArg(block); + handlerStart->bbHeapSsaPhiFunc = + new (m_pCompiler) BasicBlock::HeapPhiArg(block->bbHeapSsaNumOut); } else { -#ifdef DEBUG - BasicBlock::HeapPhiArg* curArg = handlerStart->bbHeapSsaPhiFunc; - while (curArg != nullptr) - { - assert(curArg->m_predBB != block); - curArg = curArg->m_nextArg; - } -#endif // DEBUG - handlerStart->bbHeapSsaPhiFunc = - new (m_pCompiler) BasicBlock::HeapPhiArg(block, handlerStart->bbHeapSsaPhiFunc); + // This path has a potential to introduce redundant phi args, due to multiple + // preds of the same try-begin block having the same live-out heap def, and/or + // due to nested try-begins each having preds with the same live-out heap def. + // Avoid doing quadratic processing on handler phis, and instead live with the + // occasional redundancy. + handlerStart->bbHeapSsaPhiFunc = new (m_pCompiler) + BasicBlock::HeapPhiArg(block->bbHeapSsaNumOut, handlerStart->bbHeapSsaPhiFunc); } DBG_SSA_JITDUMP(" Added phi arg for Heap from BB%02u in BB%02u.\n", block->bbNum, handlerStart->bbNum); -- 2.7.4