Trying to reduce the discussion to one e-mail thread and I picked this one.
:-)
TBH, I'm not sure what the right solution is. What I do know is that the
special cases are starting to get nuts. I see a three possible options:
1. Unify the current brw_fs_nir code. This could happen a couple of ways:
a. Break the workarounds out into some helpers.
b. Have a mega convert block (or function) that just handles everything
in a general way
2. Lower the conversions to multiple conversions in NIR. This would let
us handle some of the issues such as DF -> F -> HF but not the struding
issues.
3. Just emit the conversion and handle all these cases in
lower_conversions.
I kind-of think option 3 may be the best but I'm not at all sure on that
point and I think I'd be open to any of the above.
--Jason
On Mon, Dec 10, 2018 at 2:36 AM Iago Toral <ito...@igalia.com> wrote:
> Yes that might be a good thing to do. I mean, handling it in the backend
> isn't too bad if it was only this, but we now have a lot of conversion
> conversion cases and different kinds of restrictions on different gens and
> things start getting a bit messy. If we can pull some of these to NIR I
> think that at the very least that would make the backend code a bit more
> reasonable, which is a good thing.
>
> Would you prefer that I do that before we land this or is it okay if I fix
> it up aftwewards?
>
> In the backend I was also thinking about maybe pulling some of the
> conversion logic into one of more specialized conversion helpers that deal
> with specific things, maybe conversions from integers, or conversions
> from/to larger or smalller bit-sizes, etc I would need to think if there is
> one particular way to group conversions that makes things more sane overall.
>
> Iago
>
> On Fri, 2018-12-07 at 09:34 -0600, Jason Ekstrand wrote:
>
> Now I'm wondering even more about my previous question about just
> splitting it into two instructions in NIR.
>
> On Tue, Dec 4, 2018 at 1:18 AM Iago Toral Quiroga <ito...@igalia.com>
> wrote:
>
> From: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
>
> It is not supported directly in the HW, we need to convert to a 32-bit
> type first as intermediate step.
>
> v2 (Iago): handle conversions from 64-bit integers as well
>
> Signed-off-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
> ---
> src/intel/compiler/brw_fs_nir.cpp | 42 ++++++++++++++++++++++++++++---
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7294f49ddc0..9f3d3bf9762 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -784,6 +784,19 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
> */
>
> case nir_op_f2f16:
> + /* BDW PRM, vol02, Command Reference Instructions, mov - MOVE:
> + *
> + * "There is no direct conversion from HF to DF or DF to HF.
> + * Use two instructions and F (Float) as an intermediate type.
> + */
> + if (nir_src_bit_size(instr->src[0].src) == 64) {
> + fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_F, 1);
> + inst = bld.MOV(tmp, op[0]);
> + inst->saturate = instr->dest.saturate;
> + inst = bld.MOV(result, tmp);
> + inst->saturate = instr->dest.saturate;
>
>
> I don't think you need both saturates. Just the first one should be
> sufficient as it clamps to [0, 1] which is representable in 16-bit float.
>
>
> + break;
> + }
> inst = bld.MOV(result, op[0]);
> inst->saturate = instr->dest.saturate;
> break;
> @@ -864,7 +877,32 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
> inst->saturate = instr->dest.saturate;
> break;
> }
> - /* fallthrough */
> + inst = bld.MOV(result, op[0]);
> + inst->saturate = instr->dest.saturate;
> + break;
> +
> + case nir_op_i2f16:
> + case nir_op_u2f16:
> + /* BDW PRM, vol02, Command Reference Instructions, mov - MOVE:
> + *
> + * "There is no direct conversion from HF to Q/UQ or Q/UQ to HF.
> + * Use two instructions and F (Float) or a word integer type or
> a
> + * DWord integer type as an intermediate type."
> + */
> + if (nir_src_bit_size(instr->src[0].src) == 64) {
> + brw_reg_type reg_type = instr->op == nir_op_i2f16 ?
> + BRW_REGISTER_TYPE_D : BRW_REGISTER_TYPE_UD;
> + fs_reg tmp = bld.vgrf(reg_type, 1);
> + inst = bld.MOV(tmp, op[0]);
> + inst->saturate = instr->dest.saturate;
> + inst = bld.MOV(result, tmp);
> + inst->saturate = instr->dest.saturate;
> + break;
> + }
> + inst = bld.MOV(result, op[0]);
> + inst->saturate = instr->dest.saturate;
> + break;
> +
> case nir_op_f2f32:
> case nir_op_f2i32:
> case nir_op_f2u32:
> @@ -874,8 +912,6 @@ fs_visitor::nir_emit_alu(const fs_builder &bld,
> nir_alu_instr *instr)
> case nir_op_u2u32:
> case nir_op_i2i16:
> case nir_op_u2u16:
> - case nir_op_i2f16:
> - case nir_op_u2f16:
> case nir_op_i2i8:
> case nir_op_u2u8:
> inst = bld.MOV(result, op[0]);
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
