On Mon, 2016-03-14 at 16:48 -0700, Jason Ekstrand wrote:
>
>
> On Mon, Mar 7, 2016 at 12:46 AM, Samuel Iglesias Gonsálvez
> <sigles...@igalia.com> wrote:
> From: Connor Abbott <connor.w.abb...@intel.com>
>
> v2: Use the bit-size information from the opcode information
> if defined (Iago)
>
> Signed-off-by: Iago Toral Quiroga <ito...@igalia.com>
>
> FIXME: This should be squashed into the previous commit so we
> don't break
> the build. The break happens because the python script that
> generates the
> constant folding pass does not know how to handle the sized
> types introduced
> by the previous commit until this patch, so it ends up
> generating code with
> invalid types. Keep it separated for review purposes.
> ---
> src/compiler/nir/nir_constant_expressions.h | 2 +-
> src/compiler/nir/nir_constant_expressions.py | 246
> +++++++++++++++++----------
> src/compiler/nir/nir_opt_constant_folding.c | 24 ++-
> 3 files changed, 182 insertions(+), 90 deletions(-)
>
> diff --git a/src/compiler/nir/nir_constant_expressions.h
> b/src/compiler/nir/nir_constant_expressions.h
> index 97997f2..201f278 100644
> --- a/src/compiler/nir/nir_constant_expressions.h
> +++ b/src/compiler/nir/nir_constant_expressions.h
> @@ -28,4 +28,4 @@
> #include "nir.h"
>
> nir_const_value nir_eval_const_opcode(nir_op op, unsigned
> num_components,
> - nir_const_value *src);
> + unsigned bit_size,
> nir_const_value *src);
> diff --git a/src/compiler/nir/nir_constant_expressions.py
> b/src/compiler/nir/nir_constant_expressions.py
> index 32784f6..972d281 100644
> --- a/src/compiler/nir/nir_constant_expressions.py
> +++ b/src/compiler/nir/nir_constant_expressions.py
> @@ -1,4 +1,43 @@
> #! /usr/bin/python2
> +
> +def type_has_size(type_):
> + return type_[-1:].isdigit()
> +
> +def type_sizes(type_):
> + if type_.endswith("8"):
> + return [8]
> + elif type_.endswith("16"):
> + return [16]
> + elif type_.endswith("32"):
> + return [32]
> + elif type_.endswith("64"):
> + return [64]
> + else:
> + return [32, 64]
> +
> +def type_add_size(type_, size):
> + if type_has_size(type_):
> + return type_
> + return type_ + str(size)
> +
> +def get_const_field(type_):
> + if type_ == "int32":
> + return "i"
> + if type_ == "uint32":
> + return "u"
> + if type_ == "int64":
> + return "l"
> + if type_ == "uint64":
> + return "ul"
> + if type_ == "bool32":
> + return "b"
> + if type_ == "float32":
> + return "f"
> + if type_ == "float64":
> + return "d"
> + raise Exception(str(type_))
> + assert(0)
> +
> template = """\
> /*
> * Copyright (C) 2014 Intel Corporation
> @@ -205,110 +244,140 @@ unpack_half_1x16(uint16_t u)
> }
>
> /* Some typed vector structures to make things like src0.y
> work */
> -% for type in ["float", "int", "uint", "bool"]:
> -struct ${type}_vec {
> - ${type} x;
> - ${type} y;
> - ${type} z;
> - ${type} w;
> +typedef float float32_t;
> +typedef double float64_t;
> +typedef bool bool32_t;
> +% for type in ["float", "int", "uint"]:
> +% for width in [32, 64]:
> +struct ${type}${width}_vec {
> + ${type}${width}_t x;
> + ${type}${width}_t y;
> + ${type}${width}_t z;
> + ${type}${width}_t w;
> };
> % endfor
> +% endfor
> +
> +struct bool32_vec {
> + bool x;
> + bool y;
> + bool z;
> + bool w;
> +};
>
> % for name, op in sorted(opcodes.iteritems()):
> static nir_const_value
> -evaluate_${name}(unsigned num_components, nir_const_value
> *_src)
> +evaluate_${name}(unsigned num_components, unsigned bit_size,
> + nir_const_value *_src)
> {
> nir_const_value _dst_val = { { {0, 0, 0, 0} } };
>
> - ## For each non-per-component input, create a variable
> srcN that
> - ## contains x, y, z, and w elements which are filled in
> with the
> - ## appropriately-typed values.
> - % for j in range(op.num_inputs):
> - % if op.input_sizes[j] == 0:
> - <% continue %>
> - % elif "src" + str(j) not in op.const_expr:
> - ## Avoid unused variable warnings
> - <% continue %>
> - %endif
> -
> - struct ${op.input_types[j]}_vec src${j} = {
> - % for k in range(op.input_sizes[j]):
> - % if op.input_types[j] == "bool":
> - _src[${j}].u[${k}] != 0,
> - % else:
> - _src[${j}].${op.input_types[j][:1]}[${k}],
> - % endif
> - % endfor
> - };
> - % endfor
> + switch (bit_size) {
> + % for bit_size in [32, 64]:
> + case ${bit_size}: {
> + <%
> + output_type = type_add_size(op.output_type, bit_size)
> + input_types = [type_add_size(type_, bit_size) for type_
> in op.input_types]
> + %>
> +
> + ## For each non-per-component input, create a variable
> srcN that
> + ## contains x, y, z, and w elements which are filled in
> with the
> + ## appropriately-typed values.
> + % for j in range(op.num_inputs):
> + % if op.input_sizes[j] == 0:
> + <% continue %>
> + % elif "src" + str(j) not in op.const_expr:
> + ## Avoid unused variable warnings
> + <% continue %>
> + %endif
>
> - % if op.output_size == 0:
> - ## For per-component instructions, we need to iterate
> over the
> - ## components and apply the constant expression one
> component
> - ## at a time.
> - for (unsigned _i = 0; _i < num_components; _i++) {
> - ## For each per-component input, create a variable
> srcN that
> - ## contains the value of the current (_i'th)
> component.
> - % for j in range(op.num_inputs):
> - % if op.input_sizes[j] != 0:
> - <% continue %>
> - % elif "src" + str(j) not in op.const_expr:
> - ## Avoid unused variable warnings
> - <% continue %>
> - % elif op.input_types[j] == "bool":
> - bool src${j} = _src[${j}].u[_i] != 0;
> + struct ${input_types[j]}_vec src${j} = {
> + % for k in range(op.input_sizes[j]):
> + % if input_types[j] == "bool32":
> + _src[${j}].u[${k}] != 0,
> % else:
> - ${op.input_types[j]} src${j} =
> _src[${j}].${op.input_types[j][:1]}[_i];
> +
> _src[${j}].${get_const_field(input_types[j])}[${k}],
> % endif
> % endfor
> + };
> + % endfor
> +
> + % if op.output_size == 0:
> + ## For per-component instructions, we need to
> iterate over the
> + ## components and apply the constant expression one
> component
> + ## at a time.
> + for (unsigned _i = 0; _i < num_components; _i++) {
> + ## For each per-component input, create a
> variable srcN that
> + ## contains the value of the current (_i'th)
> component.
> + % for j in range(op.num_inputs):
> + % if op.input_sizes[j] != 0:
> + <% continue %>
> + % elif "src" + str(j) not in op.const_expr:
> + ## Avoid unused variable warnings
> + <% continue %>
> + % elif input_types[j] == "bool32":
> + bool src${j} = _src[${j}].u[_i] != 0;
> + % else:
> + ${input_types[j]}_t src${j} =
> +
> _src[${j}].${get_const_field(input_types[j])}[_i];
> + % endif
> + % endfor
> +
> + ## Create an appropriately-typed variable dst and
> assign the
> + ## result of the const_expr to it. If const_expr
> already contains
> + ## writes to dst, just include const_expr
> directly.
> + % if "dst" in op.const_expr:
> + ${output_type}_t dst;
> + ${op.const_expr}
> + % else:
> + ${output_type}_t dst = ${op.const_expr};
> + % endif
> +
> + ## Store the current component of the actual
> destination to the
> + ## value of dst.
> + % if output_type == "bool32":
> + ## Sanitize the C value to a proper NIR bool
> + _dst_val.u[_i] = dst ? NIR_TRUE : NIR_FALSE;
> + % else:
> + _dst_val.${get_const_field(output_type)}[_i] =
> dst;
> + % endif
> + }
> + % else:
> + ## In the non-per-component case, create a struct
> dst with
> + ## appropriately-typed elements x, y, z, and w and
> assign the result
> + ## of the const_expr to all components of dst, or
> include the
> + ## const_expr directly if it writes to dst already.
> + struct ${output_type}_vec dst;
>
> - ## Create an appropriately-typed variable dst and
> assign the
> - ## result of the const_expr to it. If const_expr
> already contains
> - ## writes to dst, just include const_expr directly.
> % if "dst" in op.const_expr:
> - ${op.output_type} dst;
> ${op.const_expr}
> % else:
> - ${op.output_type} dst = ${op.const_expr};
> + ## Splat the value to all components. This way
> expressions which
> + ## write the same value to all components don't
> need to explicitly
> + ## write to dest. One such example is fnoise
> which has a
> + ## const_expr of 0.0f.
> + dst.x = dst.y = dst.z = dst.w = ${op.const_expr};
> % endif
>
> - ## Store the current component of the actual
> destination to the
> - ## value of dst.
> - % if op.output_type == "bool":
> - ## Sanitize the C value to a proper NIR bool
> - _dst_val.u[_i] = dst ? NIR_TRUE : NIR_FALSE;
> - % else:
> - _dst_val.${op.output_type[:1]}[_i] = dst;
> - % endif
> - }
> - % else:
> - ## In the non-per-component case, create a struct dst
> with
> - ## appropriately-typed elements x, y, z, and w and
> assign the result
> - ## of the const_expr to all components of dst, or
> include the
> - ## const_expr directly if it writes to dst already.
> - struct ${op.output_type}_vec dst;
> -
> - % if "dst" in op.const_expr:
> - ${op.const_expr}
> - % else:
> - ## Splat the value to all components. This way
> expressions which
> - ## write the same value to all components don't need
> to explicitly
> - ## write to dest. One such example is fnoise which
> has a
> - ## const_expr of 0.0f.
> - dst.x = dst.y = dst.z = dst.w = ${op.const_expr};
> + ## For each component in the destination, copy the
> value of dst to
> + ## the actual destination.
> + % for k in range(op.output_size):
> + % if output_type == "bool32":
> + ## Sanitize the C value to a proper NIR bool
> + _dst_val.u[${k}] = dst.${"xyzw"[k]} ?
> NIR_TRUE : NIR_FALSE;
> + % else:
> + _dst_val.${get_const_field(output_type)}[${k}]
> = dst.${"xyzw"[k]};
> + % endif
> + % endfor
> % endif
>
> - ## For each component in the destination, copy the
> value of dst to
> - ## the actual destination.
> - % for k in range(op.output_size):
> - % if op.output_type == "bool":
> - ## Sanitize the C value to a proper NIR bool
> - _dst_val.u[${k}] = dst.${"xyzw"[k]} ? NIR_TRUE :
> NIR_FALSE;
> - % else:
> - _dst_val.${op.output_type[:1]}[${k}] =
> dst.${"xyzw"[k]};
> - % endif
> - % endfor
> - % endif
> + break;
> + }
> + % endfor
> +
> + default:
> + unreachable("unknown bit width");
> + }
>
> return _dst_val;
> }
> @@ -316,12 +385,12 @@ evaluate_${name}(unsigned
> num_components, nir_const_value *_src)
>
> nir_const_value
> nir_eval_const_opcode(nir_op op, unsigned num_components,
> - nir_const_value *src)
> + unsigned bit_width, nir_const_value
> *src)
> {
> switch (op) {
> % for name in sorted(opcodes.iterkeys()):
> case nir_op_${name}: {
> - return evaluate_${name}(num_components, src);
> + return evaluate_${name}(num_components, bit_width,
> src);
> break;
> }
> % endfor
> @@ -333,4 +402,7 @@ nir_eval_const_opcode(nir_op op, unsigned
> num_components,
> from nir_opcodes import opcodes
> from mako.template import Template
>
> -print Template(template).render(opcodes=opcodes)
> +print Template(template).render(opcodes=opcodes,
> type_sizes=type_sizes,
> + type_has_size=type_has_size,
> + type_add_size=type_add_size,
> +
> get_const_field=get_const_field)
> diff --git a/src/compiler/nir/nir_opt_constant_folding.c
> b/src/compiler/nir/nir_opt_constant_folding.c
> index 04876a4..29905a0 100644
> --- a/src/compiler/nir/nir_opt_constant_folding.c
> +++ b/src/compiler/nir/nir_opt_constant_folding.c
> @@ -46,10 +46,23 @@ constant_fold_alu_instr(nir_alu_instr
> *instr, void *mem_ctx)
> if (!instr->dest.dest.is_ssa)
> return false;
>
> + unsigned bit_size = 0;
> + if (!(nir_op_infos[instr->op].output_type &
> NIR_ALU_TYPE_SIZE_MASK))
> + bit_size = instr->dest.dest.ssa.bit_size;
> + else
> + bit_size = nir_op_infos[instr->op].output_type &
> NIR_ALU_TYPE_SIZE_MASK;
>
>
> This isn't right. We need to look at all the unsized types and try to
> pull it from one of those. We shouldn't fall back to grabbing from
> the sized type.
Ok, so you don't like that in the case that the alu operation has a
sized destination we grab the bit-size from the opcode definition? I am
not sure I see the problem with that... isn't the opcode mandating a
specific bit-size in that case? How can the bit-size we want be
different from that?
>
> +
> for (unsigned i = 0; i <
> nir_op_infos[instr->op].num_inputs; i++) {
> if (!instr->src[i].src.is_ssa)
> return false;
>
> + if (bit_size == 0) {
> + if (!(nir_op_infos[instr->op].input_sizes[i] &
> NIR_ALU_TYPE_SIZE_MASK))
> + bit_size = instr->src[i].src.ssa->bit_size;
> + else
> + bit_size = nir_op_infos[instr->op].input_sizes[i]
> & NIR_ALU_TYPE_SIZE_MASK;
>
>
> Same here. If they don't have any unsized sources or destinations to
> grab from, we should let bit_size be zero.
But if we have an opcode with all sized 64-bit types then...
> Also, if we have multiple sources with the same unsized type, we
> should assert that the sizes match.
>
>
> + }
> +
> nir_instr *src_instr =
> instr->src[i].src.ssa->parent_instr;
>
> if (src_instr->type != nir_instr_type_load_const)
> @@ -58,24 +71,31 @@ constant_fold_alu_instr(nir_alu_instr
> *instr, void *mem_ctx)
>
> for (unsigned j = 0; j <
> nir_ssa_alu_instr_src_components(instr, i);
> j++) {
> - src[i].u[j] =
> load_const->value.u[instr->src[i].swizzle[j]];
> + if (load_const->def.bit_size == 64)
> + src[i].ul[j] =
> load_const->value.ul[instr->src[i].swizzle[j]];
> + else
> + src[i].u[j] =
> load_const->value.u[instr->src[i].swizzle[j]];
> }
>
> /* We shouldn't have any source modifiers in the
> optimization loop. */
> assert(!instr->src[i].abs && !instr->src[i].negate);
> }
>
> + if (bit_size == 0)
> + bit_size = 32;
... this default to 32 here would not be correct any more. If at this
point the bit-size is 0 (meaning that all inputs and output are sized)
then we should take the bit-size from the opcode's output type, which is
known to be sized, right?
>
> Then it'll get set here.
>
>
> +
> /* We shouldn't have any saturate modifiers in the
> optimization loop. */
> assert(!instr->dest.saturate);
>
> nir_const_value dest =
> nir_eval_const_opcode(instr->op,
> instr->dest.dest.ssa.num_components,
> - src);
> + bit_size, src);
>
> nir_load_const_instr *new_instr =
> nir_load_const_instr_create(mem_ctx,
>
> instr->dest.dest.ssa.num_components);
>
> + new_instr->def.bit_size = instr->dest.dest.ssa.bit_size;
> new_instr->value = dest;
>
> nir_instr_insert_before(&instr->instr, &new_instr->instr);
> --
> 2.7.0
>
> _______________________________________________
> 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
