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