<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jan 3, 2017 at 4:31 AM, Samuel Iglesias Gonsálvez <span dir="ltr"><<a href="mailto:siglesias@igalia.com" target="_blank">siglesias@igalia.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Mon, 2017-01-02 at 10:34 -0800, Jason Ekstrand wrote:<br>
> I've left some inline comments but this patch has gotten me thinking<br>
> about the larger question of how we want to do conversion operations<br>
> in NIR.  Ian already has patches for int64 but float16, int8, and<br>
> int16 are all coming.  If we keep coming up with one-letter prefixes<br>
> (long, half, short, char?) and doing a2b for every opcode.  We could,<br>
> but that would be very confusing.  It would be good to cut down on<br>
> the combinatorics a bit.<br>
><br>
> Here's my straw-man proposal:  For each conversion, we leave the<br>
> destination as variable bit width and keep the source fixed.  Then we<br>
> have u2f32, u2f64, u2f16, etc. and we have one opcode regardless of<br>
> destination bit width.  This doesn't completely break the<br>
> combinatorial explosion but it does let us cut it by a factor of 3 or<br>
> 4.<br>
><br>
> Another thing that Connor and I discussed was to let conversion<br>
> operations be the one exception to the "source and destination bit-<br>
> widths must match for variable bit-width operations" and add i2i and<br>
> f2f opcodes.  That would drop the number of opcodes even further but<br>
> I'm not sure how bad the special-casing would end up being.<br>
><br>
> Thoughts?<br>
><br>
<br>
<br>
</span>The first option doesn't really address the problem and feels more like<br>
a work-around, but I guess we could go with that if other people prefer<br>
that approach. Personally, I think i would try the second option. I<br>
can't tell right know the implications this would have for the special-<br>
casing in the validation of variable bit-width opcodes, etc but I guess<br>
we can just try to write that patch and see how it looks like in the<br>
end, we can always resort to u2f32 and cia if we don't like it.<br>
<br>
In any case, regardless the solution we agree on, any of these changes<br>
would require time to get them right as they touch several places in<br>
NIR, so considering that the feature freeze for the next Mesa release<br>
is around the corner (January 13th [0]) I think we also need to discuss<br>
whether we want to tackle this before or after we land Vulkan Fp64. <br>
<br>
We are leaning towards landing Vulkan Fp64 before we do these changes<br>
so we can have that in the next release and also because we need the<br>
vertex input attributes changes introduced with this series for our<br>
Haswell VA64 implementation (which we were hoping to send this week for<br>
review too, although maybe we are trying to squeeze too many things for<br>
the next release :-D)<br>
<br>
What do you think?<br></blockquote><div><br></div><div>I 100% agree.  This was mostly a discussion for the future direction.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Sam<br>
<br>
[0] <a href="https://lists.freedesktop.org/archives/mesa-dev/2016-November/13701
5.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>archives/mesa-dev/2016-<wbr>November/13701<br>
5.html</a><br>
<div class="HOEnZb"><div class="h5"><br>
<br>
> --Jason Ekstrand<br>
><br>
> On Fri, Dec 16, 2016 at 6:49 AM, Juan A. Suarez Romero <jasuarez@igal<br>
> <a href="http://ia.com" rel="noreferrer" target="_blank">ia.com</a>> wrote:<br>
> > From: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>><br>
> ><br>
> > This function returns the nir_op corresponding to the conversion<br>
> > between<br>
> > the given nir_alu_type arguments.<br>
> ><br>
> > This function lacks support for integer-based types with bit_size<br>
> > != 32<br>
> > and for float16 conversion ops.<br>
> ><br>
> > Signed-off-by: Samuel Iglesias Gonsálvez <<a href="mailto:siglesias@igalia.com">siglesias@igalia.com</a>><br>
> > ---<br>
> >  src/compiler/nir/nir.c | 83<br>
> > ++++++++++++++++++++++++++++++<wbr>++++++++++++++++++++<br>
> >  src/compiler/nir/nir.h |  2 ++<br>
> >  2 files changed, 85 insertions(+)<br>
> ><br>
> > diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c<br>
> > index 2d882f7..3e00452 100644<br>
> > --- a/src/compiler/nir/nir.c<br>
> > +++ b/src/compiler/nir/nir.c<br>
> > @@ -1953,3 +1953,86 @@<br>
> > nir_system_value_from_<wbr>intrinsic(nir_intrinsic_op intrin)<br>
> >        unreachable("intrinsic doesn't produce a system value");<br>
> >     }<br>
> >  }<br>
> > +<br>
> > +nir_op<br>
> > +nir_type_conversion_op(nir_<wbr>alu_type src, nir_alu_type dst)<br>
> > +{<br>
> > +   nir_alu_type src_base_type = (nir_alu_type)<br>
> > nir_alu_type_get_base_type(<wbr>src);<br>
> > +   nir_alu_type dst_base_type = (nir_alu_type)<br>
> > nir_alu_type_get_base_type(<wbr>dst);<br>
> > +   unsigned src_bitsize = nir_alu_type_get_type_size(<wbr>src);<br>
> > +   unsigned dst_bitsize = nir_alu_type_get_type_size(<wbr>dst);<br>
> > +<br>
> > +   if (src_base_type == dst_base_type) {<br>
> > +      if (src_bitsize == dst_bitsize)<br>
> > +         return (src_base_type == nir_type_float) ? nir_op_fmov :<br>
> > nir_op_imov;<br>
> > +<br>
> > +      switch (src_base_type) {<br>
> > +      case nir_type_bool:<br>
> > +      case nir_type_uint:<br>
> > +      case nir_type_int:<br>
> > +         return nir_op_imov;<br>
><br>
> These three cases can't happen right now.  We should just assert that<br>
> it's float.<br>
>  <br>
> > +      case nir_type_float:<br>
> > +         /* TODO: implement support for float16 */<br>
> > +         assert(src_bitsize == 64 || dst_bitsize == 64);<br>
> > +         return (src_bitsize == 64) ? nir_op_d2f : nir_op_f2d;<br>
> > +      default:<br>
> > +         assert(!"Invalid conversion");<br>
> > +      };<br>
> > +   }<br>
> > +<br>
> > +   /* Different base type but same bit_size */<br>
> > +   if (src_bitsize == dst_bitsize) {<br>
> > +      /* TODO: This does not include specific conversions between<br>
> > +       * signed or unsigned integer types of bit size different of<br>
> > 32 yet.<br>
> > +       */<br>
> > +      assert(src_bitsize == 32);<br>
> > +      switch (src_base_type) {<br>
> > +      case nir_type_uint:<br>
> > +         return (dst_base_type == nir_type_float) ? nir_op_u2f :<br>
> > nir_op_imov;<br>
> > +      case nir_type_int:<br>
> > +         return (dst_base_type == nir_type_float) ? nir_op_i2f :<br>
> > nir_op_imov;<br>
> > +      case nir_type_bool:<br>
> > +         return (dst_base_type == nir_type_float) ? nir_op_b2f :<br>
> > nir_op_imov;<br>
><br>
> There's also a b2i opcode we should handle...<br>
>  <br>
> > +      case nir_type_float:<br>
> > +         return (dst_base_type == nir_type_uint) ? nir_op_f2u :<br>
> > +            (dst_base_type == nir_type_bool) ? nir_op_f2b :<br>
> > nir_op_f2i;<br>
><br>
> Switch?<br>
>  <br>
> > +      default:<br>
> > +         assert(!"Invalid conversion");<br>
> > +      };<br>
> > +   }<br>
> > +<br>
> > +   /* Different bit_size and different base type */<br>
> > +   /* TODO: Implement integer support for types with bit_size !=<br>
> > 32 */<br>
> > +   switch (src_base_type) {<br>
> > +   case nir_type_uint:<br>
> > +      assert(dst == nir_type_float64);<br>
> > +      return nir_op_u2d;<br>
> > +   case nir_type_int:<br>
> > +      assert(dst == nir_type_float64);<br>
> > +      return nir_op_i2d;<br>
> > +   case nir_type_bool:<br>
> > +      assert(dst == nir_type_float64);<br>
> > +      return nir_op_u2d;<br>
> > +   case nir_type_float:<br>
> > +      assert(src_bitsize == 32 || src_bitsize == 64);<br>
> > +      if (src_bitsize != 64) {<br>
> > +         assert(dst == nir_type_float64);<br>
> > +         return nir_op_f2d;<br>
> > +      }<br>
> > +      assert(dst_bitsize == 32);<br>
> > +      switch (dst_base_type) {<br>
> > +      case nir_type_uint:<br>
> > +         return nir_op_d2u;<br>
> > +      case nir_type_int:<br>
> > +         return nir_op_d2i;<br>
> > +      case nir_type_bool:<br>
> > +         return nir_op_d2b;<br>
> > +      case nir_type_float:<br>
> > +         return nir_op_d2f;<br>
> > +      default:<br>
> > +         assert(!"Invalid conversion");<br>
> > +      };<br>
> > +   default:<br>
> > +      assert(!"Invalid conversion");<br>
> > +   };<br>
> > +}<br>
> > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h<br>
> > index 9310dab..9f3abb7 100644<br>
> > --- a/src/compiler/nir/nir.h<br>
> > +++ b/src/compiler/nir/nir.h<br>
> > @@ -689,6 +689,8 @@ nir_get_nir_type_for_glsl_<wbr>type(const struct<br>
> > glsl_type *type)<br>
> >     unreachable("unknown type");<br>
> >  }<br>
> ><br>
> > +nir_op nir_type_conversion_op(nir_<wbr>alu_type src, nir_alu_type dst);<br>
> > +<br>
> >  typedef enum {<br>
> >     NIR_OP_IS_COMMUTATIVE = (1 << 0),<br>
> >     NIR_OP_IS_ASSOCIATIVE = (1 << 1),<br>
> > --<br>
> > 2.9.3<br>
> ><br>
> > ______________________________<wbr>_________________<br>
> > mesa-dev mailing list<br>
> > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
> ><br>
><br>
> ______________________________<wbr>_________________<br>
> mesa-dev mailing list<br>
> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a></div></div></blockquote></div><br></div></div>