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

[Yang, Rong R]

Pushed, thanks.

> -----Original Message-----
> From: Beignet [mailto:beignet-bounces at lists.freedesktop.org] On Behalf Of
> Pan, Xiuli
> Sent: Tuesday, April 26, 2016 14:40
> To: Song, Ruiling <ruiling.song at intel.com>; beignet at lists.freedesktop.org
> Cc: Song, Ruiling <ruiling.song at intel.com>
> Subject: Re: [Beignet] [PATCH] GBE: Fix destination grf register type for cmp
> instruction.
> 
> 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
> _______________________________________________
> Beignet mailing list
> Beignet at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/beignet