On Tue, Jan 3, 2017 at 4:31 AM, Samuel Iglesias Gonsálvez < sigles...@igalia.com> wrote:
> 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? > I 100% agree. This was mostly a discussion for the future direction. > Sam > > [0] https://lists.freedesktop.org/archives/mesa-dev/2016-November/13701 > 5.html > > > > --Jason Ekstrand > > > > On Fri, Dec 16, 2016 at 6:49 AM, Juan A. Suarez Romero <jasuarez@igal > > ia.com> wrote: > > > From: Samuel Iglesias Gonsálvez <sigles...@igalia.com> > > > > > > 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 <sigles...@igalia.com> > > > --- > > > 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 > > > mesa-dev@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
