[Beignet] [PATCH] GBE: Fix destination grf register type for cmp instruction.

[Pan, Xiuli]

LGTM!
Thanks!

-----Original Message-----
From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of Ruiling Song
Sent: Friday, April 22, 2016 2:27 PM
To: beignet at lists.freedesktop.org
Cc: Song, Ruiling <ruiling.song at intel.com>
Subject: [Beignet] [PATCH] GBE: Fix destination grf register type for cmp instruction.

Hardware have strict type requirement on cmp destination.
below (src : dst) type mapping for 'cmp' is allowed by hardware

 B,W,D,F : F
 HF      : HF
 DF      : DF
 Q       : Q

Signed-off-by: Ruiling Song <ruiling.song at intel.com>
---
 backend/src/backend/gen_insn_selection.cpp |  9 +++++++--  backend/src/backend/gen_reg_allocation.cpp | 23 +++++++++++++++++++----
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/backend/src/backend/gen_insn_selection.cpp b/backend/src/backend/gen_insn_selection.cpp
index 4f06014..fee6900 100644
--- a/backend/src/backend/gen_insn_selection.cpp
+++ b/backend/src/backend/gen_insn_selection.cpp
@@ -144,6 +144,7 @@ namespace gbe
       case GEN_TYPE_UL: return TYPE_U64;
       case GEN_TYPE_F: return TYPE_FLOAT;
       case GEN_TYPE_DF: return TYPE_DOUBLE;
+      case GEN_TYPE_HF : return TYPE_HALF;
       default: NOT_SUPPORTED; return TYPE_FLOAT;
     }
   }
@@ -4710,9 +4711,13 @@ namespace gbe
       if (liveOut.contains(dst) || dag.computeBool)
         needStoreBool = true;
 
+      // why we set the tmpDst to null?
+      // because for the listed type compare instruction could not
+      // generate bool(uw) result to grf directly, we need an extra
+      // select to generate the bool value to grf
       if(type == TYPE_S64 || type == TYPE_U64 ||
          type == TYPE_DOUBLE || type == TYPE_FLOAT ||
-         type == TYPE_U32 ||  type == TYPE_S32 /*||
+         type == TYPE_U32 ||  type == TYPE_S32 || type == TYPE_HALF 
+ /*||
          (!needStoreBool)*/)
         tmpDst = GenRegister::retype(GenRegister::null(), GEN_TYPE_F);
       else
@@ -4745,7 +4750,7 @@ namespace gbe
         } else {
           if((type == TYPE_S64 || type == TYPE_U64 ||
               type == TYPE_DOUBLE || type == TYPE_FLOAT ||
-              type == TYPE_U32 ||  type == TYPE_S32))
+              type == TYPE_U32 ||  type == TYPE_S32 || type == 
+ TYPE_HALF))
             sel.curr.flagGen = 1;
           else if (sel.isScalarReg(dst)) {
             // If the dest reg is a scalar bool, we can't set it as diff --git a/backend/src/backend/gen_reg_allocation.cpp b/backend/src/backend/gen_reg_allocation.cpp
index eaf4070..24f0b61 100644
--- a/backend/src/backend/gen_reg_allocation.cpp
+++ b/backend/src/backend/gen_reg_allocation.cpp
@@ -439,6 +439,12 @@ namespace gbe
   #define GET_FLAG_REG(insn) GenRegister::uwxgrf(IS_SCALAR_FLAG(insn) ? 1 : 8,\
                                                  ir::Register(insn.state.flagIndex));
   #define IS_TEMP_FLAG(insn) (insn.state.flag == 0 && insn.state.subFlag == 1)
+  #define NEED_DST_GRF_TYPE_FIX(ty) \
+          (ty == GEN_TYPE_F ||      \
+           ty == GEN_TYPE_HF ||     \
+           ty == GEN_TYPE_DF ||     \
+           ty == GEN_TYPE_UL ||     \
+           ty == GEN_TYPE_L)
   // Flag is a virtual flag, this function is to validate the virtual flag
   // to a physical flag. It is used to validate both temporary flag and the
   // non-temporary flag registers.
@@ -648,19 +654,28 @@ namespace gbe
             if (insn.state.predicate != GEN_PREDICATE_NONE)
               validateFlag(selection, insn);
           }
-          // This is a CMP for a pure flag booleans, we don't need to write result to
-          // the grf. And latter, we will not allocate grf for it.
           if (insn.opcode == SEL_OP_CMP &&
               (flagBooleans.contains(insn.dst(0).reg()) ||
                GenRegister::isNull(insn.dst(0)))) {
+            // This is a CMP for a pure flag booleans, we don't need to write result to
+            // the grf. And latter, we will not allocate grf for it.
             // set a temporary register to avoid switch in this block.
             bool isSrc = false;
             bool needMov = false;
             ir::Type ir_type = ir::TYPE_FLOAT;
-            if (insn.src(0).isint64() || insn.src(0).isdf())
-              ir_type = ir::TYPE_U64;
+
+            // below (src : dst) type mapping for 'cmp'
+            // is allowed by hardware
+            // B,W,D,F : F
+            // HF      : HF
+            // DF      : DF
+            // Q       : Q
+            if (NEED_DST_GRF_TYPE_FIX(insn.src(0).type))
+              ir_type = getIRType(insn.src(0).type);
+
             this->replaceReg(selection, &insn, 0, isSrc, ir_type, needMov);
           }
+
           // If the instruction requires to generate (CMP for long/int/float..)
           // the flag value to the register, and it's not a pure flag boolean,
           // we need to use SEL instruction to generate the flag value to the UW8
--
2.4.1

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