[Beignet] [PATCH] GBE: fixed the undefined phi value's liveness analysis.

[Yang, Rong R]

LGTM, thanks.

-----Original Message-----
From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of Zhigang Gong
Sent: Thursday, April 10, 2014 2:37 PM
To: beignet at lists.freedesktop.org
Cc: Gong, Zhigang
Subject: [Beignet] [PATCH] GBE: fixed the undefined phi value's liveness analysis.

If a phi component is undef from one of the predecessors, we should not pass it as the predecessor's liveout registers.
Otherwise, that phi register's liveness may be extent to the basic block zero which is not good.

Signed-off-by: Zhigang Gong <zhigang.gong at intel.com>
---
 backend/src/ir/context.hpp            |  2 ++
 backend/src/ir/function.hpp           |  1 +
 backend/src/ir/liveness.cpp           |  8 ++++----
 backend/src/ir/value.cpp              |  2 ++
 backend/src/llvm/llvm_gen_backend.cpp | 16 ++++++++++++----
 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/backend/src/ir/context.hpp b/backend/src/ir/context.hpp index 3c4ff97..ae5783a 100644
--- a/backend/src/ir/context.hpp
+++ b/backend/src/ir/context.hpp
@@ -53,6 +53,8 @@ namespace ir {
     INLINE Unit &getUnit(void) { return unit; }
     /*! Get the current processed function */
     Function &getFunction(void);
+    /*! Get the current processed block */
+    BasicBlock *getBlock(void) { return bb; }
     /*! Set the SIMD width of the function */
     void setSimdWidth(uint32_t width) const {
       GBE_ASSERT(width == 8 || width == 16); diff --git a/backend/src/ir/function.hpp b/backend/src/ir/function.hpp index abefa54..8831d47 100644
--- a/backend/src/ir/function.hpp
+++ b/backend/src/ir/function.hpp
@@ -81,6 +81,7 @@ namespace ir {
         functor(*curr);
       }
     }
+    set <Register> undefPhiRegs;
   private:
     friend class Function; //!< Owns the basic blocks
     BlockSet predecessors; //!< Incoming blocks diff --git a/backend/src/ir/liveness.cpp b/backend/src/ir/liveness.cpp index 9be539f..474ed3e 100644
--- a/backend/src/ir/liveness.cpp
+++ b/backend/src/ir/liveness.cpp
@@ -89,8 +89,10 @@ namespace ir {
       for (auto prev : currInfo->bb.getPredecessorSet()) {
         BlockInfo *prevInfo = liveness[prev];
         for (auto currInVar : currInfo->upwardUsed) {
-          auto changed = prevInfo->liveOut.insert(currInVar);
-          if (changed.second) isChanged = true;
+          if (!prevInfo->bb.undefPhiRegs.contains(currInVar)) {
+            auto changed = prevInfo->liveOut.insert(currInVar);
+            if (changed.second) isChanged = true;
+          }
         }
         if (isChanged )
           workSet.insert(prevInfo);
@@ -116,7 +118,6 @@ namespace ir {
     });
 #endif
    }
-
 /*
   As we run in SIMD mode with prediction mask to indicate active lanes.
   If a vreg is defined in a loop, and there are som uses of the vreg out of the loop, @@ -127,7 +128,6 @@ namespace ir {
   killed period, and the instructions before kill point were re-executed with different prediction,
   the inactive lanes of vreg maybe over-written. Then the out-of-loop use will got wrong data.
 */
-
   void Liveness::computeExtraLiveInOut(void) {
     const vector<Loop *> &loops = fn.getLoops();
     if(loops.size() == 0) return;
diff --git a/backend/src/ir/value.cpp b/backend/src/ir/value.cpp index 11eb0a2..1dbd4f4 100644
--- a/backend/src/ir/value.cpp
+++ b/backend/src/ir/value.cpp
@@ -97,6 +97,8 @@ namespace ir {
     // Iterate over all the predecessors
     const auto &preds = bb.getPredecessorSet();
     for (const auto &pred : preds) {
+      if (pred->undefPhiRegs.contains(reg))
+        continue;
       RegDefSet &predDef = this->getDefSet(pred, reg);
       for (auto def : predDef) udChain.insert(def);
     }
diff --git a/backend/src/llvm/llvm_gen_backend.cpp b/backend/src/llvm/llvm_gen_backend.cpp
index b46e991..a4b2c17 100644
--- a/backend/src/llvm/llvm_gen_backend.cpp
+++ b/backend/src/llvm/llvm_gen_backend.cpp
@@ -1052,15 +1052,15 @@ namespace gbe
     for (BasicBlock::iterator I = succ->begin(); isa<PHINode>(I); ++I) {
       PHINode *PN = cast<PHINode>(I);
       Value *IV = PN->getIncomingValueForBlock(curr);
+      Type *llvmType = PN->getType();
+      const ir::Type type = getType(ctx, llvmType);
+      Value *PHICopy = this->getPHICopy(PN);
+      const ir::Register dst = this->getRegister(PHICopy);
       if (!isa<UndefValue>(IV)) {
-        Type *llvmType = PN->getType();
-        const ir::Type type = getType(ctx, llvmType);
 
         // Emit the MOV required by the PHI function. We do it simple and do not
         // try to optimize them. A next data flow analysis pass on the Gen IR
         // will remove them
-        Value *PHICopy = this->getPHICopy(PN);
-        const ir::Register dst = this->getRegister(PHICopy);
         Constant *CP = dyn_cast<Constant>(IV);
         if (CP) {
           GBE_ASSERT(isa<GlobalValue>(CP) == false); @@ -1075,6 +1075,14 @@ namespace gbe
           const ir::Register src = this->getRegister(IV);
           ctx.MOV(type, dst, src);
         }
+        assert(!ctx.getBlock()->undefPhiRegs.contains(dst));
+      } else {
+        // If this is an undefined value, we don't need emit phi copy here.
+        // But we need to record it. As latter, at liveness's backward analysis,
+        // we don't need to pass the phi value/register to this BB which the phi
+        // value is undefined. Otherwise, the phi value's liveness will be extent
+        // incorrectly and may be extent to the basic block zero which is really bad.
+        ctx.getBlock()->undefPhiRegs.insert(dst);
       }
     }
   }
--
1.8.3.2

_______________________________________________
Beignet mailing list
Beignet at lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/beignet