From: Joseph Tremoulet <jotrem@microsoft.com>
Date: Fri, 30 Dec 2016 16:43:32 +0000 (-0800)
Subject: Simplify HeapPhiArg
X-Git-Tag: accepted/tizen/base/20180629.140029~2634^2
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=6fc16186bd71d97e422f7c527dfa529c0b94bd5e;p=platform%2Fupstream%2Fcoreclr.git

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.
---

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);