Pushed, thanks.
> -----Original Message----- > From: Beignet [mailto:[email protected]] On Behalf Of > Pan, Xiuli > Sent: Tuesday, April 26, 2016 14:40 > To: Song, Ruiling <[email protected]>; [email protected] > Cc: Song, Ruiling <[email protected]> > Subject: Re: [Beignet] [PATCH] GBE: Fix destination grf register type for cmp > instruction. > > LGTM! > Thanks! > > -----Original Message----- > From: Beignet [mailto:[email protected]] On Behalf Of > Ruiling Song > Sent: Friday, April 22, 2016 2:27 PM > To: [email protected] > Cc: Song, Ruiling <[email protected]> > 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 <[email protected]> > --- > 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 > [email protected] > https://lists.freedesktop.org/mailman/listinfo/beignet > _______________________________________________ > Beignet mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/beignet _______________________________________________ Beignet mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/beignet
