[Mesa-dev] [PATCH 13/14] nir: propagate bitsize information in nir_search
Iago Toral
itoral at igalia.com
Tue Mar 15 07:41:38 UTC 2016
On Mon, 2016-03-14 at 17:33 -0700, Jason Ekstrand wrote:
>
>
> On Mon, Mar 7, 2016 at 12:46 AM, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com> wrote:
> From: Connor Abbott <connor.w.abbott at intel.com>
>
> When we replace an expresion we have to compute bitsize
> information for the
> replacement. We do this in two passes to validate that bitsize
> information
> is consistent and correct: first we propagate bitsize from
> child nodes to
> parent, then we do it the other way around, starting from the
> original's
> instruction destination bitsize.
>
> v2 (Iago):
> - Always use nir_type_bool32 instead of nir_type_bool when
> generating
> algebraic optimizations. Before we used nir_type_bool32 with
> constants
> and nir_type_bool with variables.
> - Fix bool comparisons in nir_search.c to account for bitsized
> types.
>
> v3 (Sam):
> - Unpack the double constant value as unsigned long long (8
> bytes) in
> nir_algrebraic.py.
>
> Signed-off-by: Iago Toral Quiroga <itoral at igalia.com>
> Signed-off-by: Samuel Iglesias Gonsálvez
> <siglesias at igalia.com>
> ---
> src/compiler/nir/nir_algebraic.py | 22 +++-
> src/compiler/nir/nir_search.c | 244
> ++++++++++++++++++++++++++++++++++----
> src/compiler/nir/nir_search.h | 8 +-
> 3 files changed, 247 insertions(+), 27 deletions(-)
>
> diff --git a/src/compiler/nir/nir_algebraic.py
> b/src/compiler/nir/nir_algebraic.py
> index 2357b57..1818877 100644
> --- a/src/compiler/nir/nir_algebraic.py
> +++ b/src/compiler/nir/nir_algebraic.py
> @@ -63,11 +63,11 @@ class Value(object):
> static const ${val.c_type} ${val.name} = {
> { ${val.type_enum} },
> % if isinstance(val, Constant):
> - { ${hex(val)} /* ${val.value} */ },
> + ${val.type()}, { ${hex(val)} /* ${val.value} */ },
> % elif isinstance(val, Variable):
> ${val.index}, /* ${val.var_name} */
> ${'true' if val.is_constant else 'false'},
> - nir_type_${ val.required_type or 'invalid' },
> + ${val.type() or 'nir_type_invalid' },
> % elif isinstance(val, Expression):
> nir_op_${val.opcode},
> { ${', '.join(src.c_ptr for src in val.sources)} },
> @@ -107,10 +107,18 @@ class Constant(Value):
> if isinstance(self.value, (int, long)):
> return hex(self.value)
> elif isinstance(self.value, float):
> - return hex(struct.unpack('I', struct.pack('f',
> self.value))[0])
> + return hex(struct.unpack('Q', struct.pack('d',
> self.value))[0])
> else:
> assert False
>
> + def type(self):
> + if isinstance(self.value, (bool)):
> + return "nir_type_bool32"
> + elif isinstance(self.value, (int, long)):
> + return "nir_type_int"
> + elif isinstance(self.value, float):
> + return "nir_type_float"
> +
> _var_name_re = re.compile(r"(?P<const>#)?(?P<name>\w
> +)(?:@(?P<type>\w+))?")
>
> class Variable(Value):
> @@ -129,6 +137,14 @@ class Variable(Value):
>
> self.index = varset[self.var_name]
>
> + def type(self):
> + if self.required_type == 'bool':
> + return "nir_type_bool32"
> + elif self.required_type in ('int', 'unsigned'):
> + return "nir_type_int"
> + elif self.required_type == 'float':
> + return "nir_type_float"
> +
> class Expression(Value):
> def __init__(self, expr, name_base, varset):
> Value.__init__(self, name_base, "expression")
> diff --git a/src/compiler/nir/nir_search.c
> b/src/compiler/nir/nir_search.c
> index f509ce6..e874c79 100644
> --- a/src/compiler/nir/nir_search.c
> +++ b/src/compiler/nir/nir_search.c
> @@ -62,7 +62,8 @@ alu_instr_is_bool(nir_alu_instr *instr)
> case nir_op_inot:
> return src_is_bool(instr->src[0].src);
> default:
> - return nir_op_infos[instr->op].output_type ==
> nir_type_bool;
> + return (nir_op_infos[instr->op].output_type &
> NIR_ALU_TYPE_BASE_TYPE_MASK)
> + == nir_type_bool;
> }
> }
>
> @@ -125,8 +126,10 @@ match_value(const nir_search_value
> *value, nir_alu_instr *instr, unsigned src,
> nir_alu_instr *src_alu =
>
> nir_instr_as_alu(instr->src[src].src.ssa->parent_instr);
>
> - if (nir_op_infos[src_alu->op].output_type !=
> var->type &&
> - !(var->type == nir_type_bool &&
> alu_instr_is_bool(src_alu)))
> + if ((nir_op_infos[src_alu->op].output_type &
> + NIR_ALU_TYPE_BASE_TYPE_MASK) != var->type &&
> + !((var->type & NIR_ALU_TYPE_BASE_TYPE_MASK)
> == nir_type_bool &&
> + alu_instr_is_bool(src_alu)))
> return false;
> }
>
> @@ -158,21 +161,65 @@ match_value(const nir_search_value
> *value, nir_alu_instr *instr, unsigned src,
> nir_load_const_instr *load =
>
> nir_instr_as_load_const(instr->src[src].src.ssa->parent_instr);
>
> - switch (nir_op_infos[instr->op].input_types[src]) {
> + switch (const_val->type) {
> case nir_type_float:
> for (unsigned i = 0; i < num_components; ++i) {
> - if (load->value.f[new_swizzle[i]] !=
> const_val->data.f)
> + double val;
> + switch (load->def.bit_size) {
> + case 32:
> + val = load->value.f[new_swizzle[i]];
> + break;
> + case 64:
> + val = load->value.d[new_swizzle[i]];
> + break;
> + default:
> + unreachable("unknown bit size");
> + }
> +
> + if (val != const_val->data.d)
> return false;
> }
> return true;
> +
> case nir_type_int:
> + for (unsigned i = 0; i < num_components; ++i) {
> + int64_t val;
> + switch (load->def.bit_size) {
> + case 32:
> + val = load->value.i[new_swizzle[i]];
> + break;
> + case 64:
> + val = load->value.l[new_swizzle[i]];
> + break;
> + default:
> + unreachable("unknown bit size");
> + }
> +
> + if (val != const_val->data.i)
> + return false;
> + }
> + return true;
> +
> case nir_type_uint:
> - case nir_type_bool:
> + case nir_type_bool32:
> for (unsigned i = 0; i < num_components; ++i) {
> - if (load->value.i[new_swizzle[i]] !=
> const_val->data.i)
> + uint64_t val;
> + switch (load->def.bit_size) {
> + case 32:
> + val = load->value.u[new_swizzle[i]];
> + break;
> + case 64:
> + val = load->value.ul[new_swizzle[i]];
> + break;
> + default:
> + unreachable("unknown bit size");
> + }
> +
> + if (val != const_val->data.u)
> return false;
>
>
> 64-bit. Also, we may want bool to be its own case. I think keeping
> it with uint is probably fine.
Actually, const_val->data.u is 64-bit (and the same for
const_val->data.i in the case above). The definition is a bit later in
this same patch:
(...)
> diff --git a/src/compiler/nir/nir_search.h
> b/src/compiler/nir/nir_search.h
> index 7d47792..321d6d0 100644
> --- a/src/compiler/nir/nir_search.h
> +++ b/src/compiler/nir/nir_search.h
> @@ -71,10 +71,12 @@ typedef struct {
> typedef struct {
> nir_search_value value;
>
> + nir_alu_type type;
> +
> union {
> - uint32_t u;
> - int32_t i;
> - float f;
> + uint64_t u;
> + int64_t i;
> + double d;
> } data;
> } nir_search_constant;
Maybe we should rename these to u64, i64 as f64 like you suggested for a
similar case in another patch?
More information about the mesa-dev
mailing list