On Tue, Mar 15, 2016 at 12:41 AM, Iago Toral <ito...@igalia.com> wrote:
> 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
> > <sigles...@igalia.com> wrote:
> > From: Connor Abbott <connor.w.abb...@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 <ito...@igalia.com>
> > Signed-off-by: Samuel Iglesias Gonsálvez
> > <sigles...@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:
>
Right. I think this was actually a stray comment. I meant to take it out
after I'd read through the whole patch. I think it's fine as-is. No need
for explicit bit sizes if it's all 64-bit all the time.
--Jason
>
> (...)
> > 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?
>
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
