[Mesa-dev] [PATCH v3 12/43] i965/fs: Add brw_reg_type_from_bit_size utility method
Chema Casanova
jmcasanova at igalia.com
Thu Nov 2 15:18:13 UTC 2017
El 30/10/17 a las 23:15, Jason Ekstrand escribió:
>
> On Mon, Oct 30, 2017 at 3:08 PM, Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>> wrote:
>
> On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo
> <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>> wrote:
>
> From: Alejandro Piñeiro <apinheiro at igalia.com
> <mailto:apinheiro at igalia.com>>
>
> Returns the brw_type for a given ssa.bit_size, and a reference
> type.
> So if bit_size is 64, and the reference type is
> BRW_REGISTER_TYPE_F,
> it returns BRW_REGISTER_TYPE_DF. The same applies if bit_size
> is 32
> and reference type is BRW_REGISTER_TYPE_HF it returns
> BRW_REGISTER_TYPE_F
>
> v2 (Jason Ekstrand):
> - Use better unreachable() messages
> - Add Q types
>
> Signed-off-by: Jose Maria Casanova Crespo
> <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>>
> Signed-off-by: Alejandro Piñeiro <apinheiro at igalia.com
> <mailto:apinheiro at igalia.com>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>
> ---
> src/intel/compiler/brw_fs_nir.cpp | 69
> ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7ed44f534c..affe65d5e9 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -227,6 +227,65 @@ fs_visitor::nir_emit_system_values()
> }
> }
>
> +/*
> + * Returns a type based on a reference_type (word, float,
> half-float) and a
> + * given bit_size.
> + *
> + * Reference BRW_REGISTER_TYPE are HF,F,DF,W,D,UW,UD.
> + *
> + * @FIXME: 64-bit return types are always DF on integer types
> to maintain
> + * compability with uses of DF previously to the introduction
> of int64
> + * support.
>
>
> I just read this comment and I really don't like it. This is going to
> come back to bite us if we don't fix it some better way. How many
> places do we actually need to override to DF? I suppose we'll need it
> for intrinsics and a couple of ALU operations such as bcsel. I'd like
> to keep it as contained as we can.
We have doubts about this behavior also that was the reason of the
@fixme. We created this function as we were using a similar switch in 4
places when giving the 16bit_storage support over the 64bits. You can
check the uses at the end of the patch. Two of them were originally
BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF the others where as expected
BRW_REGISTER_TYPE_F : BRW_REGISTER_TYPE_DF.
As Q types weren't used in this cases that was the reason to return DF
and avoid doing a retype with its conditional if needed, to not change
original code.
In any case, i will check if there are any regressions for this cases
changing the return types.
Thanks for the review.
Chema
>
> --Jason
>
>
> + */
> +static brw_reg_type
> +brw_reg_type_from_bit_size(const unsigned bit_size,
> + const brw_reg_type reference_type)
> +{
> + switch(reference_type) {
> + case BRW_REGISTER_TYPE_HF:
> + case BRW_REGISTER_TYPE_F:
> + case BRW_REGISTER_TYPE_DF:
> + switch(bit_size) {
> + case 16:
> + return BRW_REGISTER_TYPE_HF;
> + case 32:
> + return BRW_REGISTER_TYPE_F;
> + case 64:
> + return BRW_REGISTER_TYPE_DF;
> + default:
> + unreachable("Invalid bit size");
> + }
> + case BRW_REGISTER_TYPE_W:
> + case BRW_REGISTER_TYPE_D:
> + case BRW_REGISTER_TYPE_Q:
> + switch(bit_size) {
> + case 16:
> + return BRW_REGISTER_TYPE_W;
> + case 32:
> + return BRW_REGISTER_TYPE_D;
> + case 64:
> + return BRW_REGISTER_TYPE_DF;
>
>
> This should be BRW_REGISTER_TYPE_Q
>
>
> + default:
> + unreachable("Invalid bit size");
> + }
> + case BRW_REGISTER_TYPE_UW:
> + case BRW_REGISTER_TYPE_UD:
> + case BRW_REGISTER_TYPE_UQ:
> + switch(bit_size) {
> + case 16:
> + return BRW_REGISTER_TYPE_UW;
> + case 32:
> + return BRW_REGISTER_TYPE_UD;
> + case 64:
> + return BRW_REGISTER_TYPE_DF;
>
>
> This should be BRW_REGISTER_TYPE_UQ
>
> With those fixed,
>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>
>
>
> + default:
> + unreachable("Invalid bit size");
> + }
> + default:
> + unreachable("Unknown type");
> + }
> +}
> +
> void
> fs_visitor::nir_emit_impl(nir_function_impl *impl)
> {
> @@ -240,7 +299,7 @@
> fs_visitor::nir_emit_impl(nir_function_impl *impl)
> reg->num_array_elems == 0 ? 1 : reg->num_array_elems;
> unsigned size = array_elems * reg->num_components;
> const brw_reg_type reg_type =
> - reg->bit_size == 32 ? BRW_REGISTER_TYPE_F :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(reg->bit_size,
> BRW_REGISTER_TYPE_F);
> nir_locals[reg->index] = bld.vgrf(reg_type, size);
> }
>
> @@ -1341,7 +1400,7 @@ fs_visitor::nir_emit_load_const(const
> fs_builder &bld,
> nir_load_const_instr *instr)
> {
> const brw_reg_type reg_type =
> - instr->def.bit_size == 32 ? BRW_REGISTER_TYPE_D :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(instr->def.bit_size,
> BRW_REGISTER_TYPE_D);
> fs_reg reg = bld.vgrf(reg_type, instr->def.num_components);
>
> switch (instr->def.bit_size) {
> @@ -1369,8 +1428,8 @@ fs_visitor::get_nir_src(const nir_src &src)
> fs_reg reg;
> if (src.is_ssa) {
> if (src.ssa->parent_instr->type ==
> nir_instr_type_ssa_undef) {
> - const brw_reg_type reg_type = src.ssa->bit_size == 32 ?
> - BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF;
> + const brw_reg_type reg_type =
> + brw_reg_type_from_bit_size(src.ssa->bit_size,
> BRW_REGISTER_TYPE_D);
> reg = bld.vgrf(reg_type, src.ssa->num_components);
> } else {
> reg = nir_ssa_values[src.ssa->index];
> @@ -1404,7 +1463,7 @@ fs_visitor::get_nir_dest(const nir_dest
> &dest)
> {
> if (dest.is_ssa) {
> const brw_reg_type reg_type =
> - dest.ssa.bit_size == 32 ? BRW_REGISTER_TYPE_F :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(dest.ssa.bit_size,
> BRW_REGISTER_TYPE_F);
> nir_ssa_values[dest.ssa.index] =
> bld.vgrf(reg_type, dest.ssa.num_components);
> return nir_ssa_values[dest.ssa.index];
> --
> 2.13.6
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> <mailto:mesa-dev at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>
>
>
> On Mon, Oct 30, 2017 at 3:08 PM, Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>> wrote:
>
> On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo
> <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>> wrote:
>
> From: Alejandro Piñeiro <apinheiro at igalia.com
> <mailto:apinheiro at igalia.com>>
>
> Returns the brw_type for a given ssa.bit_size, and a reference
> type.
> So if bit_size is 64, and the reference type is
> BRW_REGISTER_TYPE_F,
> it returns BRW_REGISTER_TYPE_DF. The same applies if bit_size
> is 32
> and reference type is BRW_REGISTER_TYPE_HF it returns
> BRW_REGISTER_TYPE_F
>
> v2 (Jason Ekstrand):
> - Use better unreachable() messages
> - Add Q types
>
> Signed-off-by: Jose Maria Casanova Crespo
> <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>>
> Signed-off-by: Alejandro Piñeiro <apinheiro at igalia.com
> <mailto:apinheiro at igalia.com>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>
> ---
> src/intel/compiler/brw_fs_nir.cpp | 69
> ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7ed44f534c..affe65d5e9 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -227,6 +227,65 @@ fs_visitor::nir_emit_system_values()
> }
> }
>
> +/*
> + * Returns a type based on a reference_type (word, float,
> half-float) and a
> + * given bit_size.
> + *
> + * Reference BRW_REGISTER_TYPE are HF,F,DF,W,D,UW,UD.
> + *
> + * @FIXME: 64-bit return types are always DF on integer types
> to maintain
> + * compability with uses of DF previously to the introduction
> of int64
> + * support.
> + */
> +static brw_reg_type
> +brw_reg_type_from_bit_size(const unsigned bit_size,
> + const brw_reg_type reference_type)
> +{
> + switch(reference_type) {
> + case BRW_REGISTER_TYPE_HF:
> + case BRW_REGISTER_TYPE_F:
> + case BRW_REGISTER_TYPE_DF:
> + switch(bit_size) {
> + case 16:
> + return BRW_REGISTER_TYPE_HF;
> + case 32:
> + return BRW_REGISTER_TYPE_F;
> + case 64:
> + return BRW_REGISTER_TYPE_DF;
> + default:
> + unreachable("Invalid bit size");
> + }
> + case BRW_REGISTER_TYPE_W:
> + case BRW_REGISTER_TYPE_D:
> + case BRW_REGISTER_TYPE_Q:
> + switch(bit_size) {
> + case 16:
> + return BRW_REGISTER_TYPE_W;
> + case 32:
> + return BRW_REGISTER_TYPE_D;
> + case 64:
> + return BRW_REGISTER_TYPE_DF;
>
>
> This should be BRW_REGISTER_TYPE_Q
>
>
> + default:
> + unreachable("Invalid bit size");
> + }
> + case BRW_REGISTER_TYPE_UW:
> + case BRW_REGISTER_TYPE_UD:
> + case BRW_REGISTER_TYPE_UQ:
> + switch(bit_size) {
> + case 16:
> + return BRW_REGISTER_TYPE_UW;
> + case 32:
> + return BRW_REGISTER_TYPE_UD;
> + case 64:
> + return BRW_REGISTER_TYPE_DF;
>
>
> This should be BRW_REGISTER_TYPE_UQ
>
> With those fixed,
>
> Reviewed-by: Jason Ekstrand <jason at jlekstrand.net
> <mailto:jason at jlekstrand.net>>
>
>
> + default:
> + unreachable("Invalid bit size");
> + }
> + default:
> + unreachable("Unknown type");
> + }
> +}
> +
> void
> fs_visitor::nir_emit_impl(nir_function_impl *impl)
> {
> @@ -240,7 +299,7 @@
> fs_visitor::nir_emit_impl(nir_function_impl *impl)
> reg->num_array_elems == 0 ? 1 : reg->num_array_elems;
> unsigned size = array_elems * reg->num_components;
> const brw_reg_type reg_type =
> - reg->bit_size == 32 ? BRW_REGISTER_TYPE_F :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(reg->bit_size,
> BRW_REGISTER_TYPE_F);
> nir_locals[reg->index] = bld.vgrf(reg_type, size);
> }
>
> @@ -1341,7 +1400,7 @@ fs_visitor::nir_emit_load_const(const
> fs_builder &bld,
> nir_load_const_instr *instr)
> {
> const brw_reg_type reg_type =
> - instr->def.bit_size == 32 ? BRW_REGISTER_TYPE_D :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(instr->def.bit_size,
> BRW_REGISTER_TYPE_D);
> fs_reg reg = bld.vgrf(reg_type, instr->def.num_components);
>
> switch (instr->def.bit_size) {
> @@ -1369,8 +1428,8 @@ fs_visitor::get_nir_src(const nir_src &src)
> fs_reg reg;
> if (src.is_ssa) {
> if (src.ssa->parent_instr->type ==
> nir_instr_type_ssa_undef) {
> - const brw_reg_type reg_type = src.ssa->bit_size == 32 ?
> - BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_DF;
> + const brw_reg_type reg_type =
> + brw_reg_type_from_bit_size(src.ssa->bit_size,
> BRW_REGISTER_TYPE_D);
> reg = bld.vgrf(reg_type, src.ssa->num_components);
> } else {
> reg = nir_ssa_values[src.ssa->index];
> @@ -1404,7 +1463,7 @@ fs_visitor::get_nir_dest(const nir_dest
> &dest)
> {
> if (dest.is_ssa) {
> const brw_reg_type reg_type =
> - dest.ssa.bit_size == 32 ? BRW_REGISTER_TYPE_F :
> BRW_REGISTER_TYPE_DF;
> + brw_reg_type_from_bit_size(dest.ssa.bit_size,
> BRW_REGISTER_TYPE_F);
> nir_ssa_values[dest.ssa.index] =
> bld.vgrf(reg_type, dest.ssa.num_components);
> return nir_ssa_values[dest.ssa.index];
> --
> 2.13.6
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> <mailto:mesa-dev at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list