[v3,02/42] intel/compiler: add a NIR pass to lower conversions

Submitted by Iago Toral Quiroga on Jan. 15, 2019, 1:53 p.m.

Details

Message ID 20190115135414.2313-3-itoral@igalia.com
State New
Headers show
Series "intel: VK_KHR_shader_float16_int8 implementation" ( rev: 5 4 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga Jan. 15, 2019, 1:53 p.m.
Some conversions are not directly supported in hardware and need to be
split in two conversion instructions going through an intermediary type.
Doing this at the NIR level simplifies a bit the complexity in the backend.

v2:
 - Consider fp16 rounding conversion opcodes
 - Properly handle swizzles on conversion sources.

Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com> (v1)
---
 src/intel/Makefile.sources                    |   1 +
 src/intel/compiler/brw_nir.c                  |   1 +
 src/intel/compiler/brw_nir.h                  |   2 +
 .../compiler/brw_nir_lower_conversions.c      | 158 ++++++++++++++++++
 src/intel/compiler/meson.build                |   1 +
 5 files changed, 163 insertions(+)
 create mode 100644 src/intel/compiler/brw_nir_lower_conversions.c

Patch hide | download patch | download mbox

diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
index 94a28d370e8..9975daa3ad1 100644
--- a/src/intel/Makefile.sources
+++ b/src/intel/Makefile.sources
@@ -83,6 +83,7 @@  COMPILER_FILES = \
 	compiler/brw_nir_analyze_boolean_resolves.c \
 	compiler/brw_nir_analyze_ubo_ranges.c \
 	compiler/brw_nir_attribute_workarounds.c \
+	compiler/brw_nir_lower_conversions.c \
 	compiler/brw_nir_lower_cs_intrinsics.c \
 	compiler/brw_nir_lower_image_load_store.c \
 	compiler/brw_nir_lower_mem_access_bit_sizes.c \
diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index 92d7fe4bede..572ab824a94 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -882,6 +882,7 @@  brw_postprocess_nir(nir_shader *nir, const struct brw_compiler *compiler,
    OPT(nir_opt_move_comparisons);
 
    OPT(nir_lower_bool_to_int32);
+   OPT(brw_nir_lower_conversions);
 
    OPT(nir_lower_locals_to_regs);
 
diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h
index bc81950d47e..662b2627e95 100644
--- a/src/intel/compiler/brw_nir.h
+++ b/src/intel/compiler/brw_nir.h
@@ -114,6 +114,8 @@  void brw_nir_lower_tcs_outputs(nir_shader *nir, const struct brw_vue_map *vue,
                                GLenum tes_primitive_mode);
 void brw_nir_lower_fs_outputs(nir_shader *nir);
 
+bool brw_nir_lower_conversions(nir_shader *nir);
+
 bool brw_nir_lower_image_load_store(nir_shader *nir,
                                     const struct gen_device_info *devinfo);
 void brw_nir_rewrite_image_intrinsic(nir_intrinsic_instr *intrin,
diff --git a/src/intel/compiler/brw_nir_lower_conversions.c b/src/intel/compiler/brw_nir_lower_conversions.c
new file mode 100644
index 00000000000..583167c7753
--- /dev/null
+++ b/src/intel/compiler/brw_nir_lower_conversions.c
@@ -0,0 +1,158 @@ 
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "brw_nir.h"
+#include "compiler/nir/nir_builder.h"
+
+static nir_op
+get_conversion_op(nir_alu_type src_type,
+                  unsigned src_bit_size,
+                  nir_alu_type dst_type,
+                  unsigned dst_bit_size,
+                  nir_rounding_mode rounding_mode)
+{
+   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
+   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
+
+   return nir_type_conversion_op(src_full_type, dst_full_type, rounding_mode);
+}
+
+static nir_rounding_mode
+get_opcode_rounding_mode(nir_op op)
+{
+   switch (op) {
+   case nir_op_f2f16_rtz:
+      return nir_rounding_mode_rtz;
+   case nir_op_f2f16_rtne:
+      return nir_rounding_mode_rtne;
+   default:
+      return nir_rounding_mode_undef;
+   }
+}
+
+static void
+split_conversion(nir_builder *b, nir_alu_instr *alu, nir_op op1, nir_op op2)
+{
+   b->cursor = nir_before_instr(&alu->instr);
+   assert(alu->dest.write_mask == 1);
+   nir_ssa_def *src = nir_ssa_for_alu_src(b, alu, 0);
+   nir_ssa_def *tmp = nir_build_alu(b, op1, src, NULL, NULL, NULL);
+   nir_ssa_def *res = nir_build_alu(b, op2, tmp, NULL, NULL, NULL);
+   nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa, nir_src_for_ssa(res));
+   nir_instr_remove(&alu->instr);
+}
+
+static bool
+lower_instr(nir_builder *b, nir_alu_instr *alu)
+{
+   unsigned src_bit_size = nir_src_bit_size(alu->src[0].src);
+   nir_alu_type src_type = nir_op_infos[alu->op].input_types[0];
+   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
+
+   unsigned dst_bit_size = nir_dest_bit_size(alu->dest.dest);
+   nir_alu_type dst_type = nir_op_infos[alu->op].output_type;
+   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
+
+   /* 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.
+    *
+    *    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 ((src_full_type == nir_type_float16 && dst_bit_size == 64) ||
+       (src_bit_size == 64 && dst_full_type == nir_type_float16)) {
+      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
+                                     nir_rounding_mode_undef);
+      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
+                                     get_opcode_rounding_mode(alu->op));
+      split_conversion(b, alu, op1, op2);
+      return true;
+   }
+
+   /* SKL PRM, vol 02a, Command Reference: Instructions, Move:
+    *
+    *   "There is no direct conversion from B/UB to DF or DF to B/UB. Use
+    *    two instructions and a word or DWord intermediate type."
+    *
+    *   "There is no direct conversion from B/UB to Q/UQ or Q/UQ to B/UB.
+    *    Use two instructions and a word or DWord intermediate integer
+    *    type."
+    */
+   if ((src_bit_size == 8 && dst_bit_size == 64) ||
+       (src_bit_size == 64 && dst_bit_size == 8)) {
+      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
+                                     nir_rounding_mode_undef);
+      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
+                                     nir_rounding_mode_undef);
+      split_conversion(b, alu, op1, op2);
+      return true;
+   }
+
+   return false;
+}
+
+static bool
+lower_impl(nir_function_impl *impl)
+{
+   nir_builder b;
+   nir_builder_init(&b, impl);
+   bool progress = false;
+
+   nir_foreach_block(block, impl) {
+      nir_foreach_instr_safe(instr, block) {
+         if (instr->type != nir_instr_type_alu)
+            continue;
+
+         nir_alu_instr *alu = nir_instr_as_alu(instr);
+         assert(alu->dest.dest.is_ssa);
+
+         if (nir_op_infos[alu->op].num_inputs > 1)
+            continue;
+
+         progress = lower_instr(&b, alu) || progress;
+      }
+   }
+
+   if (progress) {
+      nir_metadata_preserve(impl, nir_metadata_block_index |
+                                  nir_metadata_dominance);
+   }
+
+   return progress;
+}
+
+bool
+brw_nir_lower_conversions(nir_shader *shader)
+{
+   bool progress = false;
+
+   nir_foreach_function(function, shader) {
+      if (function->impl)
+         progress |= lower_impl(function->impl);
+   }
+
+   return progress;
+}
diff --git a/src/intel/compiler/meson.build b/src/intel/compiler/meson.build
index f8e5e2518fe..9daf2a94260 100644
--- a/src/intel/compiler/meson.build
+++ b/src/intel/compiler/meson.build
@@ -76,6 +76,7 @@  libintel_compiler_files = files(
   'brw_nir_analyze_boolean_resolves.c',
   'brw_nir_analyze_ubo_ranges.c',
   'brw_nir_attribute_workarounds.c',
+  'brw_nir_lower_conversions.c',
   'brw_nir_lower_cs_intrinsics.c',
   'brw_nir_lower_image_load_store.c',
   'brw_nir_lower_mem_access_bit_sizes.c',

Comments

On Tue, Jan 15, 2019 at 7:54 AM Iago Toral Quiroga <itoral@igalia.com>
wrote:

> Some conversions are not directly supported in hardware and need to be
> split in two conversion instructions going through an intermediary type.
> Doing this at the NIR level simplifies a bit the complexity in the backend.
>
> v2:
>  - Consider fp16 rounding conversion opcodes
>  - Properly handle swizzles on conversion sources.
>
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com> (v1)
> ---
>  src/intel/Makefile.sources                    |   1 +
>  src/intel/compiler/brw_nir.c                  |   1 +
>  src/intel/compiler/brw_nir.h                  |   2 +
>  .../compiler/brw_nir_lower_conversions.c      | 158 ++++++++++++++++++
>  src/intel/compiler/meson.build                |   1 +
>  5 files changed, 163 insertions(+)
>  create mode 100644 src/intel/compiler/brw_nir_lower_conversions.c
>
> diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
> index 94a28d370e8..9975daa3ad1 100644
> --- a/src/intel/Makefile.sources
> +++ b/src/intel/Makefile.sources
> @@ -83,6 +83,7 @@ COMPILER_FILES = \
>         compiler/brw_nir_analyze_boolean_resolves.c \
>         compiler/brw_nir_analyze_ubo_ranges.c \
>         compiler/brw_nir_attribute_workarounds.c \
> +       compiler/brw_nir_lower_conversions.c \
>         compiler/brw_nir_lower_cs_intrinsics.c \
>         compiler/brw_nir_lower_image_load_store.c \
>         compiler/brw_nir_lower_mem_access_bit_sizes.c \
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index 92d7fe4bede..572ab824a94 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -882,6 +882,7 @@ brw_postprocess_nir(nir_shader *nir, const struct
> brw_compiler *compiler,
>     OPT(nir_opt_move_comparisons);
>
>     OPT(nir_lower_bool_to_int32);
> +   OPT(brw_nir_lower_conversions);
>

This is *really* late and I don't think you actually want this to run after
we apply source/destination modifiers.  If you just move it to right after
nir_opt_algebraic_late, that will fix a multitude of issues.


>
>     OPT(nir_lower_locals_to_regs);
>
> diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h
> index bc81950d47e..662b2627e95 100644
> --- a/src/intel/compiler/brw_nir.h
> +++ b/src/intel/compiler/brw_nir.h
> @@ -114,6 +114,8 @@ void brw_nir_lower_tcs_outputs(nir_shader *nir, const
> struct brw_vue_map *vue,
>                                 GLenum tes_primitive_mode);
>  void brw_nir_lower_fs_outputs(nir_shader *nir);
>
> +bool brw_nir_lower_conversions(nir_shader *nir);
> +
>  bool brw_nir_lower_image_load_store(nir_shader *nir,
>                                      const struct gen_device_info
> *devinfo);
>  void brw_nir_rewrite_image_intrinsic(nir_intrinsic_instr *intrin,
> diff --git a/src/intel/compiler/brw_nir_lower_conversions.c
> b/src/intel/compiler/brw_nir_lower_conversions.c
> new file mode 100644
> index 00000000000..583167c7753
> --- /dev/null
> +++ b/src/intel/compiler/brw_nir_lower_conversions.c
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "brw_nir.h"
> +#include "compiler/nir/nir_builder.h"
> +
> +static nir_op
> +get_conversion_op(nir_alu_type src_type,
> +                  unsigned src_bit_size,
> +                  nir_alu_type dst_type,
> +                  unsigned dst_bit_size,
> +                  nir_rounding_mode rounding_mode)
> +{
> +   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
> +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
> +
> +   return nir_type_conversion_op(src_full_type, dst_full_type,
> rounding_mode);
> +}
> +
> +static nir_rounding_mode
> +get_opcode_rounding_mode(nir_op op)
> +{
> +   switch (op) {
> +   case nir_op_f2f16_rtz:
> +      return nir_rounding_mode_rtz;
> +   case nir_op_f2f16_rtne:
> +      return nir_rounding_mode_rtne;
> +   default:
> +      return nir_rounding_mode_undef;
> +   }
> +}
> +
> +static void
> +split_conversion(nir_builder *b, nir_alu_instr *alu, nir_op op1, nir_op
> op2)
> +{
> +   b->cursor = nir_before_instr(&alu->instr);
> +   assert(alu->dest.write_mask == 1);
> +   nir_ssa_def *src = nir_ssa_for_alu_src(b, alu, 0);
>

Hrm...  Given how late this pass runs, I'm not a fan of using
nir_ssa_for_alu_src because it might insert a MOV and we don't copy-prop
afterwards.  Instead, we could assert that the destination is SSA and do

b->cursor = nir_after_instr(&alu->instr);
alu->op = op1;
alu->dest.bit_size =
nir_alu_type_get_type_size(nir_op_infos[alu->op].output_type);
nir_ssa_def *new_res = nir_build_alu(b, op2, &alu->dest.dest.ssa, NULL,
NULL, NULL);
nir_ssa_def_rewrite_uses_after(&alu->dest.dest.ssa,
nir_src_for_ssa(new_res),
                                            new_res->parent_instr);

If you don't feel comfortable whacking the opcode in the original
instruction, then just make a new one and build it directly with the old
instruction's ALU src.  Or, we could just move where it's run.  (see my
comment above.)


> +   nir_ssa_def *tmp = nir_build_alu(b, op1, src, NULL, NULL, NULL);
> +   nir_ssa_def *res = nir_build_alu(b, op2, tmp, NULL, NULL, NULL);
> +   nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa, nir_src_for_ssa(res));
> +   nir_instr_remove(&alu->instr);
> +}
> +
> +static bool
> +lower_instr(nir_builder *b, nir_alu_instr *alu)
> +{
> +   unsigned src_bit_size = nir_src_bit_size(alu->src[0].src);
> +   nir_alu_type src_type = nir_op_infos[alu->op].input_types[0];
> +   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
> +
> +   unsigned dst_bit_size = nir_dest_bit_size(alu->dest.dest);
> +   nir_alu_type dst_type = nir_op_infos[alu->op].output_type;
>
+   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
>

The nir_op_infos[*].output_type will already be a full type.  Maybe just
delete the dst_type temporary so that no one gets confused and thinks it's
a base type.


> +
> +   /* 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.
> +    *
> +    *    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 ((src_full_type == nir_type_float16 && dst_bit_size == 64) ||
> +       (src_bit_size == 64 && dst_full_type == nir_type_float16)) {
> +      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
> +                                     nir_rounding_mode_undef);
> +      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
> +                                     get_opcode_rounding_mode(alu->op));
> +      split_conversion(b, alu, op1, op2);
> +      return true;
> +   }
> +
> +   /* SKL PRM, vol 02a, Command Reference: Instructions, Move:
> +    *
> +    *   "There is no direct conversion from B/UB to DF or DF to B/UB. Use
> +    *    two instructions and a word or DWord intermediate type."
> +    *
> +    *   "There is no direct conversion from B/UB to Q/UQ or Q/UQ to B/UB.
> +    *    Use two instructions and a word or DWord intermediate integer
> +    *    type."
> +    */
> +   if ((src_bit_size == 8 && dst_bit_size == 64) ||
> +       (src_bit_size == 64 && dst_bit_size == 8)) {
> +      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
> +                                     nir_rounding_mode_undef);
> +      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
> +                                     nir_rounding_mode_undef);
> +      split_conversion(b, alu, op1, op2);
> +      return true;
> +   }
> +
> +   return false;
> +}
> +
> +static bool
> +lower_impl(nir_function_impl *impl)
> +{
> +   nir_builder b;
> +   nir_builder_init(&b, impl);
> +   bool progress = false;
> +
> +   nir_foreach_block(block, impl) {
> +      nir_foreach_instr_safe(instr, block) {
> +         if (instr->type != nir_instr_type_alu)
> +            continue;
> +
> +         nir_alu_instr *alu = nir_instr_as_alu(instr);
> +         assert(alu->dest.dest.is_ssa);
> +
> +         if (nir_op_infos[alu->op].num_inputs > 1)
>

I don't think this is a reliable way to detect conversion opcodes.  There
are many unary operations that aren't conversions.  You're only getting
lucky because there are non which do float16 <-> 64=bit or 8-bit <->
64-bit.  I've thought many times about throwing a boolean in nir_op_infos
for is_conversion.  Maybe now is a good time to do it?


> +            continue;
> +
> +         progress = lower_instr(&b, alu) || progress;
> +      }
> +   }
> +
> +   if (progress) {
> +      nir_metadata_preserve(impl, nir_metadata_block_index |
> +                                  nir_metadata_dominance);
> +   }
> +
> +   return progress;
> +}
> +
> +bool
> +brw_nir_lower_conversions(nir_shader *shader)
> +{
> +   bool progress = false;
> +
> +   nir_foreach_function(function, shader) {
> +      if (function->impl)
> +         progress |= lower_impl(function->impl);
> +   }
> +
> +   return progress;
> +}
> diff --git a/src/intel/compiler/meson.build
> b/src/intel/compiler/meson.build
> index f8e5e2518fe..9daf2a94260 100644
> --- a/src/intel/compiler/meson.build
> +++ b/src/intel/compiler/meson.build
> @@ -76,6 +76,7 @@ libintel_compiler_files = files(
>    'brw_nir_analyze_boolean_resolves.c',
>    'brw_nir_analyze_ubo_ranges.c',
>    'brw_nir_attribute_workarounds.c',
> +  'brw_nir_lower_conversions.c',
>    'brw_nir_lower_cs_intrinsics.c',
>    'brw_nir_lower_image_load_store.c',
>    'brw_nir_lower_mem_access_bit_sizes.c',
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Thu, 2019-01-17 at 13:42 -0600, Jason Ekstrand wrote:
> On Tue, Jan 15, 2019 at 7:54 AM Iago Toral Quiroga <itoral@igalia.com
> > wrote:
> > Some conversions are not directly supported in hardware and need to
> > be
> > 
> > split in two conversion instructions going through an intermediary
> > type.
> > 
> > Doing this at the NIR level simplifies a bit the complexity in the
> > backend.
> > 
> > 
> > 
> > v2:
> > 
> >  - Consider fp16 rounding conversion opcodes
> > 
> >  - Properly handle swizzles on conversion sources.
> > 
> > 
> > 
> > Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com> (v1)
> > 
> > ---
> > 
> >  src/intel/Makefile.sources                    |   1 +
> > 
> >  src/intel/compiler/brw_nir.c                  |   1 +
> > 
> >  src/intel/compiler/brw_nir.h                  |   2 +
> > 
> >  .../compiler/brw_nir_lower_conversions.c      | 158
> > ++++++++++++++++++
> > 
> >  src/intel/compiler/meson.build                |   1 +
> > 
> >  5 files changed, 163 insertions(+)
> > 
> >  create mode 100644 src/intel/compiler/brw_nir_lower_conversions.c
> > 
> > 
> > 
> > diff --git a/src/intel/Makefile.sources
> > b/src/intel/Makefile.sources
> > 
> > index 94a28d370e8..9975daa3ad1 100644
> > 
> > --- a/src/intel/Makefile.sources
> > 
> > +++ b/src/intel/Makefile.sources
> > 
> > @@ -83,6 +83,7 @@ COMPILER_FILES = \
> > 
> >         compiler/brw_nir_analyze_boolean_resolves.c \
> > 
> >         compiler/brw_nir_analyze_ubo_ranges.c \
> > 
> >         compiler/brw_nir_attribute_workarounds.c \
> > 
> > +       compiler/brw_nir_lower_conversions.c \
> > 
> >         compiler/brw_nir_lower_cs_intrinsics.c \
> > 
> >         compiler/brw_nir_lower_image_load_store.c \
> > 
> >         compiler/brw_nir_lower_mem_access_bit_sizes.c \
> > 
> > diff --git a/src/intel/compiler/brw_nir.c
> > b/src/intel/compiler/brw_nir.c
> > 
> > index 92d7fe4bede..572ab824a94 100644
> > 
> > --- a/src/intel/compiler/brw_nir.c
> > 
> > +++ b/src/intel/compiler/brw_nir.c
> > 
> > @@ -882,6 +882,7 @@ brw_postprocess_nir(nir_shader *nir, const
> > struct brw_compiler *compiler,
> > 
> >     OPT(nir_opt_move_comparisons);
> > 
> > 
> > 
> >     OPT(nir_lower_bool_to_int32);
> > 
> > +   OPT(brw_nir_lower_conversions);
> 
> This is *really* late and I don't think you actually want this to run
> after we apply source/destination modifiers.  If you just move it to
> right after nir_opt_algebraic_late, that will fix a multitude of
> issues.

Sure, I'll move it there.
> > 
> >     OPT(nir_lower_locals_to_regs);
> > 
> > 
> > 
> > diff --git a/src/intel/compiler/brw_nir.h
> > b/src/intel/compiler/brw_nir.h
> > 
> > index bc81950d47e..662b2627e95 100644
> > 
> > --- a/src/intel/compiler/brw_nir.h
> > 
> > +++ b/src/intel/compiler/brw_nir.h
> > 
> > @@ -114,6 +114,8 @@ void brw_nir_lower_tcs_outputs(nir_shader *nir,
> > const struct brw_vue_map *vue,
> > 
> >                                 GLenum tes_primitive_mode);
> > 
> >  void brw_nir_lower_fs_outputs(nir_shader *nir);
> > 
> > 
> > 
> > +bool brw_nir_lower_conversions(nir_shader *nir);
> > 
> > +
> > 
> >  bool brw_nir_lower_image_load_store(nir_shader *nir,
> > 
> >                                      const struct gen_device_info
> > *devinfo);
> > 
> >  void brw_nir_rewrite_image_intrinsic(nir_intrinsic_instr *intrin,
> > 
> > diff --git a/src/intel/compiler/brw_nir_lower_conversions.c
> > b/src/intel/compiler/brw_nir_lower_conversions.c
> > 
> > new file mode 100644
> > 
> > index 00000000000..583167c7753
> > 
> > --- /dev/null
> > 
> > +++ b/src/intel/compiler/brw_nir_lower_conversions.c
> > 
> > @@ -0,0 +1,158 @@
> > 
> > +/*
> > 
> > + * Copyright © 2018 Intel Corporation
> > 
> > + *
> > 
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > 
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > 
> > + * to deal in the Software without restriction, including without
> > limitation
> > 
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > 
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > 
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > 
> > + *
> > 
> > + * The above copyright notice and this permission notice
> > (including the next
> > 
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > 
> > + * Software.
> > 
> > + *
> > 
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > 
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > 
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > 
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > 
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > 
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > 
> > + * IN THE SOFTWARE.
> > 
> > + */
> > 
> > +
> > 
> > +#include "brw_nir.h"
> > 
> > +#include "compiler/nir/nir_builder.h"
> > 
> > +
> > 
> > +static nir_op
> > 
> > +get_conversion_op(nir_alu_type src_type,
> > 
> > +                  unsigned src_bit_size,
> > 
> > +                  nir_alu_type dst_type,
> > 
> > +                  unsigned dst_bit_size,
> > 
> > +                  nir_rounding_mode rounding_mode)
> > 
> > +{
> > 
> > +   nir_alu_type src_full_type = (nir_alu_type) (src_type |
> > src_bit_size);
> > 
> > +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type |
> > dst_bit_size);
> > 
> > +
> > 
> > +   return nir_type_conversion_op(src_full_type, dst_full_type,
> > rounding_mode);
> > 
> > +}
> > 
> > +
> > 
> > +static nir_rounding_mode
> > 
> > +get_opcode_rounding_mode(nir_op op)
> > 
> > +{
> > 
> > +   switch (op) {
> > 
> > +   case nir_op_f2f16_rtz:
> > 
> > +      return nir_rounding_mode_rtz;
> > 
> > +   case nir_op_f2f16_rtne:
> > 
> > +      return nir_rounding_mode_rtne;
> > 
> > +   default:
> > 
> > +      return nir_rounding_mode_undef;
> > 
> > +   }
> > 
> > +}
> > 
> > +
> > 
> > +static void
> > 
> > +split_conversion(nir_builder *b, nir_alu_instr *alu, nir_op op1,
> > nir_op op2)
> > 
> > +{
> > 
> > +   b->cursor = nir_before_instr(&alu->instr);
> > 
> > +   assert(alu->dest.write_mask == 1);
> > 
> > +   nir_ssa_def *src = nir_ssa_for_alu_src(b, alu, 0);
> 
> Hrm...  Given how late this pass runs, I'm not a fan of using
> nir_ssa_for_alu_src because it might insert a MOV and we don't copy-
> prop afterwards.  Instead, we could assert that the destination is
> SSA and do

I did this because I found cases where we would have things like a vec4
load and then a handful of swizzled MOVs to extract the components of
the vec4 into scalars and this helper would handle the swizzled sources
gracefully.However, I think what you suggest below should be just fine,
so I'll do that.
> b->cursor = nir_after_instr(&alu->instr);
> alu->op = op1;
> alu->dest.bit_size = nir_alu_type_get_type_size(nir_op_infos[alu-
> >op].output_type);
> nir_ssa_def *new_res = nir_build_alu(b, op2, &alu->dest.dest.ssa,
> NULL, NULL, NULL);
> nir_ssa_def_rewrite_uses_after(&alu->dest.dest.ssa,
> nir_src_for_ssa(new_res),
>                                             new_res->parent_instr);
> If you don't feel comfortable whacking the opcode in the original
> instruction, then just make a new one and build it directly with the
> old instruction's ALU src.  Or, we could just move where it's run. 
> (see my comment above.)
> 
> 
> > +   nir_ssa_def *tmp = nir_build_alu(b, op1, src, NULL, NULL,
> > NULL);
> > 
> > +   nir_ssa_def *res = nir_build_alu(b, op2, tmp, NULL, NULL,
> > NULL);
> > 
> > +   nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa,
> > nir_src_for_ssa(res));
> > 
> > +   nir_instr_remove(&alu->instr);
> > 
> > +}
> > 
> > +
> > 
> > +static bool
> > 
> > +lower_instr(nir_builder *b, nir_alu_instr *alu)
> > 
> > +{
> > 
> > +   unsigned src_bit_size = nir_src_bit_size(alu->src[0].src);
> > 
> > +   nir_alu_type src_type = nir_op_infos[alu->op].input_types[0];
> > 
> > +   nir_alu_type src_full_type = (nir_alu_type) (src_type |
> > src_bit_size);
> > 
> > +
> > 
> > +   unsigned dst_bit_size = nir_dest_bit_size(alu->dest.dest);
> > 
> > +   nir_alu_type dst_type = nir_op_infos[alu->op].output_type;
> > 
> > +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type |
> > dst_bit_size);
> 
> The nir_op_infos[*].output_type will already be a full type.  Maybe
> just delete the dst_type temporary so that no one gets confused and
> thinks it's a base type.

Right, will do that.
> > +
> > 
> > +   /* 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.
> > 
> > +    *
> > 
> > +    *    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 ((src_full_type == nir_type_float16 && dst_bit_size == 64)
> > ||
> > 
> > +       (src_bit_size == 64 && dst_full_type == nir_type_float16))
> > {
> > 
> > +      nir_op op1 = get_conversion_op(src_type, src_bit_size,
> > src_type, 32,
> > 
> > +                                     nir_rounding_mode_undef);
> > 
> > +      nir_op op2 = get_conversion_op(src_type, 32, dst_type,
> > dst_bit_size,
> > 
> > +                                     get_opcode_rounding_mode(alu-
> > >op));
> > 
> > +      split_conversion(b, alu, op1, op2);
> > 
> > +      return true;
> > 
> > +   }
> > 
> > +
> > 
> > +   /* SKL PRM, vol 02a, Command Reference: Instructions, Move:
> > 
> > +    *
> > 
> > +    *   "There is no direct conversion from B/UB to DF or DF to
> > B/UB. Use
> > 
> > +    *    two instructions and a word or DWord intermediate type."
> > 
> > +    *
> > 
> > +    *   "There is no direct conversion from B/UB to Q/UQ or Q/UQ
> > to B/UB.
> > 
> > +    *    Use two instructions and a word or DWord intermediate
> > integer
> > 
> > +    *    type."
> > 
> > +    */
> > 
> > +   if ((src_bit_size == 8 && dst_bit_size == 64) ||
> > 
> > +       (src_bit_size == 64 && dst_bit_size == 8)) {
> > 
> > +      nir_op op1 = get_conversion_op(src_type, src_bit_size,
> > src_type, 32,
> > 
> > +                                     nir_rounding_mode_undef);
> > 
> > +      nir_op op2 = get_conversion_op(src_type, 32, dst_type,
> > dst_bit_size,
> > 
> > +                                     nir_rounding_mode_undef);
> > 
> > +      split_conversion(b, alu, op1, op2);
> > 
> > +      return true;
> > 
> > +   }
> > 
> > +
> > 
> > +   return false;
> > 
> > +}
> > 
> > +
> > 
> > +static bool
> > 
> > +lower_impl(nir_function_impl *impl)
> > 
> > +{
> > 
> > +   nir_builder b;
> > 
> > +   nir_builder_init(&b, impl);
> > 
> > +   bool progress = false;
> > 
> > +
> > 
> > +   nir_foreach_block(block, impl) {
> > 
> > +      nir_foreach_instr_safe(instr, block) {
> > 
> > +         if (instr->type != nir_instr_type_alu)
> > 
> > +            continue;
> > 
> > +
> > 
> > +         nir_alu_instr *alu = nir_instr_as_alu(instr);
> > 
> > +         assert(alu->dest.dest.is_ssa);
> > 
> > +
> > 
> > +         if (nir_op_infos[alu->op].num_inputs > 1)
> 
> I don't think this is a reliable way to detect conversion opcodes. 
> There are many unary operations that aren't conversions.  You're only
> getting lucky because there are non which do float16 <-> 64=bit or 8-
> bit <-> 64-bit.  I've thought many times about throwing a boolean in
> nir_op_infos for is_conversion.  Maybe now is a good time to do it?

Yes, I knew that this was not going to only let conversions through and
was relying on what you say above. Adding an is_conversion field souns
good to me, alternatively I guess we could add a is_conversion()
helper, but I guess having the field in nir_op_infos maye be a bit
easier to maintain if we want to add more opcodes in the future.
>  
> > +            continue;
> > 
> > +
> > 
> > +         progress = lower_instr(&b, alu) || progress;
> > 
> > +      }
> > 
> > +   }
> > 
> > +
> > 
> > +   if (progress) {
> > 
> > +      nir_metadata_preserve(impl, nir_metadata_block_index |
> > 
> > +                                  nir_metadata_dominance);
> > 
> > +   }
> > 
> > +
> > 
> > +   return progress;
> > 
> > +}
> > 
> > +
> > 
> > +bool
> > 
> > +brw_nir_lower_conversions(nir_shader *shader)
> > 
> > +{
> > 
> > +   bool progress = false;
> > 
> > +
> > 
> > +   nir_foreach_function(function, shader) {
> > 
> > +      if (function->impl)
> > 
> > +         progress |= lower_impl(function->impl);
> > 
> > +   }
> > 
> > +
> > 
> > +   return progress;
> > 
> > +}
> > 
> > diff --git a/src/intel/compiler/meson.build
> > b/src/intel/compiler/meson.build
> > 
> > index f8e5e2518fe..9daf2a94260 100644
> > 
> > --- a/src/intel/compiler/meson.build
> > 
> > +++ b/src/intel/compiler/meson.build
> > 
> > @@ -76,6 +76,7 @@ libintel_compiler_files = files(
> > 
> >    'brw_nir_analyze_boolean_resolves.c',
> > 
> >    'brw_nir_analyze_ubo_ranges.c',
> > 
> >    'brw_nir_attribute_workarounds.c',
> > 
> > +  'brw_nir_lower_conversions.c',
> > 
> >    'brw_nir_lower_cs_intrinsics.c',
> > 
> >    'brw_nir_lower_image_load_store.c',
> > 
> >    'brw_nir_lower_mem_access_bit_sizes.c',
> >
On January 18, 2019 01:48:25 Iago Toral <itoral@igalia.com> wrote:
> On Thu, 2019-01-17 at 13:42 -0600, Jason Ekstrand wrote:
>> On Tue, Jan 15, 2019 at 7:54 AM Iago Toral Quiroga <itoral@igalia.com> wrote:
>>> Some conversions are not directly supported in hardware and need to be
>>> split in two conversion instructions going through an intermediary type.
>>> Doing this at the NIR level simplifies a bit the complexity in the backend.
>>>
>>> v2:
>>> - Consider fp16 rounding conversion opcodes
>>> - Properly handle swizzles on conversion sources.
>>>
>>> Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com> (v1)
>>> ---
>>> src/intel/Makefile.sources                    |   1 +
>>> src/intel/compiler/brw_nir.c                  |   1 +
>>> src/intel/compiler/brw_nir.h                  |   2 +
>>> .../compiler/brw_nir_lower_conversions.c      | 158 ++++++++++++++++++
>>> src/intel/compiler/meson.build                |   1 +
>>> 5 files changed, 163 insertions(+)
>>> create mode 100644 src/intel/compiler/brw_nir_lower_conversions.c
>>>
>>> diff --git a/src/intel/Makefile.sources b/src/intel/Makefile.sources
>>> index 94a28d370e8..9975daa3ad1 100644
>>> --- a/src/intel/Makefile.sources
>>> +++ b/src/intel/Makefile.sources
>>> @@ -83,6 +83,7 @@ COMPILER_FILES = \
>>>        compiler/brw_nir_analyze_boolean_resolves.c \
>>>        compiler/brw_nir_analyze_ubo_ranges.c \
>>>        compiler/brw_nir_attribute_workarounds.c \
>>> +       compiler/brw_nir_lower_conversions.c \
>>>        compiler/brw_nir_lower_cs_intrinsics.c \
>>>        compiler/brw_nir_lower_image_load_store.c \
>>>        compiler/brw_nir_lower_mem_access_bit_sizes.c \
>>> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
>>> index 92d7fe4bede..572ab824a94 100644
>>> --- a/src/intel/compiler/brw_nir.c
>>> +++ b/src/intel/compiler/brw_nir.c
>>> @@ -882,6 +882,7 @@ brw_postprocess_nir(nir_shader *nir, const struct 
>>> brw_compiler *compiler,
>>>    OPT(nir_opt_move_comparisons);
>>>
>>>    OPT(nir_lower_bool_to_int32);
>>> +   OPT(brw_nir_lower_conversions);
>>
>> This is *really* late and I don't think you actually want this to run after 
>> we apply source/destination modifiers.  If you just move it to right after 
>> nir_opt_algebraic_late, that will fix a multitude of issues.
>
> Sure, I'll move it there.
>
>>>
>>>    OPT(nir_lower_locals_to_regs);
>>>
>>> diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h
>>> index bc81950d47e..662b2627e95 100644
>>> --- a/src/intel/compiler/brw_nir.h
>>> +++ b/src/intel/compiler/brw_nir.h
>>> @@ -114,6 +114,8 @@ void brw_nir_lower_tcs_outputs(nir_shader *nir, const 
>>> struct brw_vue_map *vue,
>>>                                GLenum tes_primitive_mode);
>>> void brw_nir_lower_fs_outputs(nir_shader *nir);
>>>
>>> +bool brw_nir_lower_conversions(nir_shader *nir);
>>> +
>>> bool brw_nir_lower_image_load_store(nir_shader *nir,
>>>                                     const struct gen_device_info *devinfo);
>>> void brw_nir_rewrite_image_intrinsic(nir_intrinsic_instr *intrin,
>>> diff --git a/src/intel/compiler/brw_nir_lower_conversions.c 
>>> b/src/intel/compiler/brw_nir_lower_conversions.c
>>> new file mode 100644
>>> index 00000000000..583167c7753
>>> --- /dev/null
>>> +++ b/src/intel/compiler/brw_nir_lower_conversions.c
>>> @@ -0,0 +1,158 @@
>>> +/*
>>> + * Copyright © 2018 Intel Corporation
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>> + * copy of this software and associated documentation files (the "Software"),
>>> + * to deal in the Software without restriction, including without limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including the next
>>> + * paragraph) shall be included in all copies or substantial portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
>>> DEALINGS
>>> + * IN THE SOFTWARE.
>>> + */
>>> +
>>> +#include "brw_nir.h"
>>> +#include "compiler/nir/nir_builder.h"
>>> +
>>> +static nir_op
>>> +get_conversion_op(nir_alu_type src_type,
>>> +                  unsigned src_bit_size,
>>> +                  nir_alu_type dst_type,
>>> +                  unsigned dst_bit_size,
>>> +                  nir_rounding_mode rounding_mode)
>>> +{
>>> +   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
>>> +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
>>> +
>>> +   return nir_type_conversion_op(src_full_type, dst_full_type, rounding_mode);
>>> +}
>>> +
>>> +static nir_rounding_mode
>>> +get_opcode_rounding_mode(nir_op op)
>>> +{
>>> +   switch (op) {
>>> +   case nir_op_f2f16_rtz:
>>> +      return nir_rounding_mode_rtz;
>>> +   case nir_op_f2f16_rtne:
>>> +      return nir_rounding_mode_rtne;
>>> +   default:
>>> +      return nir_rounding_mode_undef;
>>> +   }
>>> +}
>>> +
>>> +static void
>>> +split_conversion(nir_builder *b, nir_alu_instr *alu, nir_op op1, nir_op op2)
>>> +{
>>> +   b->cursor = nir_before_instr(&alu->instr);
>>> +   assert(alu->dest.write_mask == 1);
>>> +   nir_ssa_def *src = nir_ssa_for_alu_src(b, alu, 0);
>>
>> Hrm...  Given how late this pass runs, I'm not a fan of using 
>> nir_ssa_for_alu_src because it might insert a MOV and we don't copy-prop 
>> afterwards.  Instead, we could assert that the destination is SSA and do
>
> I did this because I found cases where we would have things like a vec4 
> load and then a handful of swizzled MOVs to extract the components of the 
> vec4 into scalars and this helper would handle the swizzled sources 
> gracefully.However, I think what you suggest below should be just fine, so 
> I'll do that.

Just to be clear, you don't need to make any of the changes suggested here 
if you just move where we call the pass. If copy-prop runs after it, that 
will clean up the added movs.

>
>> b->cursor = nir_after_instr(&alu->instr);
>> alu->op = op1;
>> alu->dest.bit_size = 
>> nir_alu_type_get_type_size(nir_op_infos[alu->op].output_type);
>> nir_ssa_def *new_res = nir_build_alu(b, op2, &alu->dest.dest.ssa, NULL, 
>> NULL, NULL);
>> nir_ssa_def_rewrite_uses_after(&alu->dest.dest.ssa, nir_src_for_ssa(new_res),
>>                                            new_res->parent_instr);
>> If you don't feel comfortable whacking the opcode in the original 
>> instruction, then just make a new one and build it directly with the old 
>> instruction's ALU src.  Or, we could just move where it's run.  (see my 
>> comment above.)
>>
>>> +   nir_ssa_def *tmp = nir_build_alu(b, op1, src, NULL, NULL, NULL);
>>> +   nir_ssa_def *res = nir_build_alu(b, op2, tmp, NULL, NULL, NULL);
>>> +   nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa, nir_src_for_ssa(res));
>>> +   nir_instr_remove(&alu->instr);
>>> +}
>>> +
>>> +static bool
>>> +lower_instr(nir_builder *b, nir_alu_instr *alu)
>>> +{
>>> +   unsigned src_bit_size = nir_src_bit_size(alu->src[0].src);
>>> +   nir_alu_type src_type = nir_op_infos[alu->op].input_types[0];
>>> +   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
>>> +
>>> +   unsigned dst_bit_size = nir_dest_bit_size(alu->dest.dest);
>>> +   nir_alu_type dst_type = nir_op_infos[alu->op].output_type;
>>> +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
>>
>> The nir_op_infos[*].output_type will already be a full type.  Maybe just 
>> delete the dst_type temporary so that no one gets confused and thinks it's 
>> a base type.
>
> Right, will do that.
>
>>> +
>>> +   /* 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.
>>> +    *
>>> +    *    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 ((src_full_type == nir_type_float16 && dst_bit_size == 64) ||
>>> +       (src_bit_size == 64 && dst_full_type == nir_type_float16)) {
>>> +      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
>>> +                                     nir_rounding_mode_undef);
>>> +      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
>>> +                                     get_opcode_rounding_mode(alu->op));
>>> +      split_conversion(b, alu, op1, op2);
>>> +      return true;
>>> +   }
>>> +
>>> +   /* SKL PRM, vol 02a, Command Reference: Instructions, Move:
>>> +    *
>>> +    *   "There is no direct conversion from B/UB to DF or DF to B/UB. Use
>>> +    *    two instructions and a word or DWord intermediate type."
>>> +    *
>>> +    *   "There is no direct conversion from B/UB to Q/UQ or Q/UQ to B/UB.
>>> +    *    Use two instructions and a word or DWord intermediate integer
>>> +    *    type."
>>> +    */
>>> +   if ((src_bit_size == 8 && dst_bit_size == 64) ||
>>> +       (src_bit_size == 64 && dst_bit_size == 8)) {
>>> +      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
>>> +                                     nir_rounding_mode_undef);
>>> +      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
>>> +                                     nir_rounding_mode_undef);
>>> +      split_conversion(b, alu, op1, op2);
>>> +      return true;
>>> +   }
>>> +
>>> +   return false;
>>> +}
>>> +
>>> +static bool
>>> +lower_impl(nir_function_impl *impl)
>>> +{
>>> +   nir_builder b;
>>> +   nir_builder_init(&b, impl);
>>> +   bool progress = false;
>>> +
>>> +   nir_foreach_block(block, impl) {
>>> +      nir_foreach_instr_safe(instr, block) {
>>> +         if (instr->type != nir_instr_type_alu)
>>> +            continue;
>>> +
>>> +         nir_alu_instr *alu = nir_instr_as_alu(instr);
>>> +         assert(alu->dest.dest.is_ssa);
>>> +
>>> +         if (nir_op_infos[alu->op].num_inputs > 1)
>>
>> I don't think this is a reliable way to detect conversion opcodes.  There 
>> are many unary operations that aren't conversions.  You're only getting 
>> lucky because there are non which do float16 <-> 64=bit or 8-bit <-> 
>> 64-bit.  I've thought many times about throwing a boolean in nir_op_infos 
>> for is_conversion.  Maybe now is a good time to do it?
>
> Yes, I knew that this was not going to only let conversions through and was 
> relying on what you say above. Adding an is_conversion field souns good to 
> me, alternatively I guess we could add a is_conversion() helper, but I 
> guess having the field in nir_op_infos maye be a bit easier to maintain if 
> we want to add more opcodes in the future.
>
>>
>>> +            continue;
>>> +
>>> +         progress = lower_instr(&b, alu) || progress;
>>> +      }
>>> +   }
>>> +
>>> +   if (progress) {
>>> +      nir_metadata_preserve(impl, nir_metadata_block_index |
>>> +                                  nir_metadata_dominance);
>>> +   }
>>> +
>>> +   return progress;
>>> +}
>>> +
>>> +bool
>>> +brw_nir_lower_conversions(nir_shader *shader)
>>> +{
>>> +   bool progress = false;
>>> +
>>> +   nir_foreach_function(function, shader) {
>>> +      if (function->impl)
>>> +         progress |= lower_impl(function->impl);
>>> +   }
>>> +
>>> +   return progress;
>>> +}
>>> diff --git a/src/intel/compiler/meson.build b/src/intel/compiler/meson.build
>>> index f8e5e2518fe..9daf2a94260 100644
>>> --- a/src/intel/compiler/meson.build
>>> +++ b/src/intel/compiler/meson.build
>>> @@ -76,6 +76,7 @@ libintel_compiler_files = files(
>>>   'brw_nir_analyze_boolean_resolves.c',
>>>   'brw_nir_analyze_ubo_ranges.c',
>>>   'brw_nir_attribute_workarounds.c',
>>> +  'brw_nir_lower_conversions.c',
>>>   'brw_nir_lower_cs_intrinsics.c',
>>>   'brw_nir_lower_image_load_store.c',
>>>   'brw_nir_lower_mem_access_bit_sizes.c',
On Fri, 2019-01-18 at 06:46 -0600, Jason Ekstrand wrote:
> On January 18, 2019 01:48:25 Iago Toral <itoral@igalia.com> wrote:
> 
> > On Thu, 2019-01-17 at 13:42 -0600, Jason Ekstrand wrote:
> > > On Tue, Jan 15, 2019 at 7:54 AM Iago Toral Quiroga <
> > > itoral@igalia.com> wrote

(...)
> > > > +   nir_ssa_def *tmp = nir_build_alu(b, op1, src, NULL, NULL,
> > > > NULL);
> > > > 
> > > > +   nir_ssa_def *res = nir_build_alu(b, op2, tmp, NULL, NULL,
> > > > NULL);
> > > > 
> > > > +   nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa,
> > > > nir_src_for_ssa(res));
> > > > 
> > > > +   nir_instr_remove(&alu->instr);
> > > > 
> > > > +}
> > > > 
> > > > +
> > > > 
> > > > +static bool
> > > > 
> > > > +lower_instr(nir_builder *b, nir_alu_instr *alu)
> > > > 
> > > > +{
> > > > 
> > > > +   unsigned src_bit_size = nir_src_bit_size(alu->src[0].src);
> > > > 
> > > > +   nir_alu_type src_type = nir_op_infos[alu-
> > > > >op].input_types[0];
> > > > 
> > > > +   nir_alu_type src_full_type = (nir_alu_type) (src_type |
> > > > src_bit_size);
> > > > 
> > > > +
> > > > 
> > > > +   unsigned dst_bit_size = nir_dest_bit_size(alu->dest.dest);
> > > > 
> > > > +   nir_alu_type dst_type = nir_op_infos[alu->op].output_type;
> > > > 
> > > > +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type |
> > > > dst_bit_size);
> > > 
> > > The nir_op_infos[*].output_type will already be a full type. 
> > > Maybe just delete the dst_type temporary so that no one gets
> > > confused and thinks it's a base type.
> > 
> > Right, will do that.

Actually, we want the base type too, since that is what we pass to the
get_conversion_op() helper, I'll just pick it from the dst_full_type
with the nir_alu_type_get_base_type() helper.
> > > > +
> > > > 
> > > > +   /* 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.
> > > > 
> > > > +    *
> > > > 
> > > > +    *    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 ((src_full_type == nir_type_float16 && dst_bit_size ==
> > > > 64) ||
> > > > 
> > > > +       (src_bit_size == 64 && dst_full_type ==
> > > > nir_type_float16)) {
> > > > 
> > > > +      nir_op op1 = get_conversion_op(src_type, src_bit_size,
> > > > src_type, 32,
> > > > 
> > > > +                                     nir_rounding_mode_undef);
> > > > 
> > > > +      nir_op op2 = get_conversion_op(src_type, 32, dst_type,
> > > > dst_bit_size,
> > > > 
> > > > +                                   
> > > >  get_opcode_rounding_mode(alu->op));
> > > > 
> > > > +      split_conversion(b, alu, op1, op2);
> > > > 
> > > > +      return true;
> > > > 
> > > > +   }
> > > > 
> > > > +
> > > > 
> > > > +   /* SKL PRM, vol 02a, Command Reference: Instructions, Move:
> > > > 
> > > > +    *
> > > > 
> > > > +    *   "There is no direct conversion from B/UB to DF or DF
> > > > to B/UB. Use
> > > > 
> > > > +    *    two instructions and a word or DWord intermediate
> > > > type."
> > > > 
> > > > +    *
> > > > 
> > > > +    *   "There is no direct conversion from B/UB to Q/UQ or
> > > > Q/UQ to B/UB.
> > > > 
> > > > +    *    Use two instructions and a word or DWord intermediate
> > > > integer
> > > > 
> > > > +    *    type."
> > > > 
> > > > +    */
> > > > 
> > > > +   if ((src_bit_size == 8 && dst_bit_size == 64) ||
> > > > 
> > > > +       (src_bit_size == 64 && dst_bit_size == 8)) {
> > > > 
> > > > +      nir_op op1 = get_conversion_op(src_type, src_bit_size,
> > > > src_type, 32,
> > > > 
> > > > +                                     nir_rounding_mode_undef);
> > > > 
> > > > +      nir_op op2 = get_conversion_op(src_type, 32, dst_type,
> > > > dst_bit_size,
> > > > 
> > > > +                                     nir_rounding_mode_undef);
> > > > 
> > > > +      split_conversion(b, alu, op1, op2);
> > > > 
> > > > +      return true;
> > > > 
> > > > +   }
> > > > 
> > > > +
> > > > 
> > > > +   return false;
> > > > 
> > > > +}
> > > > 
> > > > +
> > > > 
> > > > +static bool
> > > > 
> > > > +lower_impl(nir_function_impl *impl)
> > > > 
> > > > +{
> > > > 
> > > > +   nir_builder b;
> > > > 
> > > > +   nir_builder_init(&b, impl);
> > > > 
> > > > +   bool progress = false;
> > > > 
> > > > +
> > > > 
> > > > +   nir_foreach_block(block, impl) {
> > > > 
> > > > +      nir_foreach_instr_safe(instr, block) {
> > > > 
> > > > +         if (instr->type != nir_instr_type_alu)
> > > > 
> > > > +            continue;
> > > > 
> > > > +
> > > > 
> > > > +         nir_alu_instr *alu = nir_instr_as_alu(instr);
> > > > 
> > > > +         assert(alu->dest.dest.is_ssa);
> > > > 
> > > > +
> > > > 
> > > > +         if (nir_op_infos[alu->op].num_inputs > 1)
> > > 
> > > I don't think this is a reliable way to detect conversion
> > > opcodes.  There are many unary operations that aren't
> > > conversions.  You're only getting lucky because there are non
> > > which do float16 <-> 64=bit or 8-bit <-> 64-bit.  I've thought
> > > many times about throwing a boolean in nir_op_infos for
> > > is_conversion.  Maybe now is a good time to do it?
> > 
> > Yes, I knew that this was not going to only let conversions through
> > and was relying on what you say above. Adding an is_conversion
> > field souns good to me, alternatively I guess we could add a
> > is_conversion() helper, but I guess having the field in
> > nir_op_infos maye be a bit easier to maintain if we want to add
> > more opcodes in the future.

Thinking about this, I guess we only want to set this to True for the
opcodes that are explicit numeric conversions, that is, the ones we
generate right after "# Generate all of the numeric conversion
opcodes". I am saying this because even if we have unop_convert(), that
is also used for things like unpack opcodes, but_count, etc that are
not real conversions. I was thinking about creating a new
unop_numeric_convert() helper and have all other helpers set this new
field to False. Sounds good?
> > > > +            continue;
> > > > 
> > > > +
> > > > 
> > > > +         progress = lower_instr(&b, alu) || progress;
> > > > 
> > > > +      }
> > > > 
> > > > +   }
> > > > 
> > > > +
> > > > 
> > > > +   if (progress) {
> > > > 
> > > > +      nir_metadata_preserve(impl, nir_metadata_block_index |
> > > > 
> > > > +                                  nir_metadata_dominance);
> > > > 
> > > > +   }
> > > > 
> > > > +
> > > > 
> > > > +   return progress;
> > > > 
> > > > +}
> > > > 
> > > > +
> > > > 
> > > > +bool
> > > > 
> > > > +brw_nir_lower_conversions(nir_shader *shader)
> > > > 
> > > > +{
> > > > 
> > > > +   bool progress = false;
> > > > 
> > > > +
> > > > 
> > > > +   nir_foreach_function(function, shader) {
> > > > 
> > > > +      if (function->impl)
> > > > 
> > > > +         progress |= lower_impl(function->impl);
> > > > 
> > > > +   }
> > > > 
> > > > +
> > > > 
> > > > +   return progress;
> > > > 
> > > > +}
> > > > 
> > > > diff --git a/src/intel/compiler/meson.build
> > > > b/src/intel/compiler/meson.build
> > > > 
> > > > index f8e5e2518fe..9daf2a94260 100644
> > > > 
> > > > --- a/src/intel/compiler/meson.build
> > > > 
> > > > +++ b/src/intel/compiler/meson.build
> > > > 
> > > > @@ -76,6 +76,7 @@ libintel_compiler_files = files(
> > > > 
> > > >    'brw_nir_analyze_boolean_resolves.c',
> > > > 
> > > >    'brw_nir_analyze_ubo_ranges.c',
> > > > 
> > > >    'brw_nir_attribute_workarounds.c',
> > > > 
> > > > +  'brw_nir_lower_conversions.c',
> > > > 
> > > >    'brw_nir_lower_cs_intrinsics.c',
> > > > 
> > > >    'brw_nir_lower_image_load_store.c',
> > > > 
> > > >    'brw_nir_lower_mem_access_bit_sizes.c',
> > > > 
> 
> 
> 
> 
> 
> 
> 
>
On Mon, Jan 21, 2019 at 2:09 AM Iago Toral <itoral@igalia.com> wrote:

> On Fri, 2019-01-18 at 06:46 -0600, Jason Ekstrand wrote:
>
> On January 18, 2019 01:48:25 Iago Toral <itoral@igalia.com> wrote:
>
> On Thu, 2019-01-17 at 13:42 -0600, Jason Ekstrand wrote:
>
> On Tue, Jan 15, 2019 at 7:54 AM Iago Toral Quiroga <itoral@igalia.com>
> wrote
>
>
> (...)
>
>
> +   nir_ssa_def *tmp = nir_build_alu(b, op1, src, NULL, NULL, NULL);
> +   nir_ssa_def *res = nir_build_alu(b, op2, tmp, NULL, NULL, NULL);
> +   nir_ssa_def_rewrite_uses(&alu->dest.dest.ssa, nir_src_for_ssa(res));
> +   nir_instr_remove(&alu->instr);
> +}
> +
> +static bool
> +lower_instr(nir_builder *b, nir_alu_instr *alu)
> +{
> +   unsigned src_bit_size = nir_src_bit_size(alu->src[0].src);
> +   nir_alu_type src_type = nir_op_infos[alu->op].input_types[0];
> +   nir_alu_type src_full_type = (nir_alu_type) (src_type | src_bit_size);
> +
> +   unsigned dst_bit_size = nir_dest_bit_size(alu->dest.dest);
> +   nir_alu_type dst_type = nir_op_infos[alu->op].output_type;
> +   nir_alu_type dst_full_type = (nir_alu_type) (dst_type | dst_bit_size);
>
>
> The nir_op_infos[*].output_type will already be a full type.  Maybe just
> delete the dst_type temporary so that no one gets confused and thinks it's
> a base type.
>
>
> Right, will do that.
>
>
> Actually, we want the base type too, since that is what we pass to the
> get_conversion_op() helper, I'll just pick it from the dst_full_type with
> the nir_alu_type_get_base_type() helper.
>

Sounds like the right thing to do.


> +
> +   /* 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.
> +    *
> +    *    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 ((src_full_type == nir_type_float16 && dst_bit_size == 64) ||
> +       (src_bit_size == 64 && dst_full_type == nir_type_float16)) {
> +      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
> +                                     nir_rounding_mode_undef);
> +      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
> +                                     get_opcode_rounding_mode(alu->op));
> +      split_conversion(b, alu, op1, op2);
> +      return true;
> +   }
> +
> +   /* SKL PRM, vol 02a, Command Reference: Instructions, Move:
> +    *
> +    *   "There is no direct conversion from B/UB to DF or DF to B/UB. Use
> +    *    two instructions and a word or DWord intermediate type."
> +    *
> +    *   "There is no direct conversion from B/UB to Q/UQ or Q/UQ to B/UB.
> +    *    Use two instructions and a word or DWord intermediate integer
> +    *    type."
> +    */
> +   if ((src_bit_size == 8 && dst_bit_size == 64) ||
> +       (src_bit_size == 64 && dst_bit_size == 8)) {
> +      nir_op op1 = get_conversion_op(src_type, src_bit_size, src_type, 32,
> +                                     nir_rounding_mode_undef);
> +      nir_op op2 = get_conversion_op(src_type, 32, dst_type, dst_bit_size,
> +                                     nir_rounding_mode_undef);
> +      split_conversion(b, alu, op1, op2);
> +      return true;
> +   }
> +
> +   return false;
> +}
> +
> +static bool
> +lower_impl(nir_function_impl *impl)
> +{
> +   nir_builder b;
> +   nir_builder_init(&b, impl);
> +   bool progress = false;
> +
> +   nir_foreach_block(block, impl) {
> +      nir_foreach_instr_safe(instr, block) {
> +         if (instr->type != nir_instr_type_alu)
> +            continue;
> +
> +         nir_alu_instr *alu = nir_instr_as_alu(instr);
> +         assert(alu->dest.dest.is_ssa);
> +
> +         if (nir_op_infos[alu->op].num_inputs > 1)
>
>
> I don't think this is a reliable way to detect conversion opcodes.  There
> are many unary operations that aren't conversions.  You're only getting
> lucky because there are non which do float16 <-> 64=bit or 8-bit <->
> 64-bit.  I've thought many times about throwing a boolean in nir_op_infos
> for is_conversion.  Maybe now is a good time to do it?
>
>
> Yes, I knew that this was not going to only let conversions through and
> was relying on what you say above. Adding an is_conversion field souns good
> to me, alternatively I guess we could add a is_conversion() helper, but I
> guess having the field in nir_op_infos maye be a bit easier to maintain if
> we want to add more opcodes in the future.
>
>
> Thinking about this, I guess we only want to set this to True for the
> opcodes that are explicit numeric conversions, that is, the ones we
> generate right after "# Generate all of the numeric conversion opcodes". I
> am saying this because even if we have unop_convert(), that is also used
> for things like unpack opcodes, but_count, etc that are not real
> conversions. I was thinking about creating a new unop_numeric_convert()
> helper and have all other helpers set this new field to False. Sounds good?
>

Seems reasonable.


> +            continue;
> +
> +         progress = lower_instr(&b, alu) || progress;
> +      }
> +   }
> +
> +   if (progress) {
> +      nir_metadata_preserve(impl, nir_metadata_block_index |
> +                                  nir_metadata_dominance);
> +   }
> +
> +   return progress;
> +}
> +
> +bool
> +brw_nir_lower_conversions(nir_shader *shader)
> +{
> +   bool progress = false;
> +
> +   nir_foreach_function(function, shader) {
> +      if (function->impl)
> +         progress |= lower_impl(function->impl);
> +   }
> +
> +   return progress;
> +}
> diff --git a/src/intel/compiler/meson.build
> b/src/intel/compiler/meson.build
> index f8e5e2518fe..9daf2a94260 100644
> --- a/src/intel/compiler/meson.build
> +++ b/src/intel/compiler/meson.build
> @@ -76,6 +76,7 @@ libintel_compiler_files = files(
>    'brw_nir_analyze_boolean_resolves.c',
>    'brw_nir_analyze_ubo_ranges.c',
>    'brw_nir_attribute_workarounds.c',
> +  'brw_nir_lower_conversions.c',
>    'brw_nir_lower_cs_intrinsics.c',
>    'brw_nir_lower_image_load_store.c',
>    'brw_nir_lower_mem_access_bit_sizes.c',
>
>
>
>