> Looks like you leak the constants? You could pass ctx->ssa_constants
> instead of NULL and the allocation would be automatically freed.
Hm, alright. Is there documentation anywhere on how memctx works in
general?
> Instead of hardcoding 4096, use impl->ssa_index?
Good catch, thank you.
> Looks like instead of encoding components here you could use
> nir_op_info->num_inputs?
Components at this point is a misnomer; it's really "encoding type". The
correct solution, now that I have the infrastructure for it, is to use a
combination of nir_op_info and instruction quirks, and get rid of the
magic numbers here. Bumping up priority list for next time I dive into
the compiler.
> > + nir_foreach_variable(var, &nir->uniforms) {
> > + if (glsl_get_base_type(var->type) == GLSL_TYPE_SAMPLER)
> > continue;
> > +
> > + unsigned length = glsl_get_aoa_size(var->type);
> > +
> > + if (!length) {
> > + length = glsl_get_length(var->type);
> > + }
> > +
> > + if (!length) {
> > + length = glsl_get_matrix_columns(var->type);
> > + }
>
> This seems suspicious -- I don't have anything like this for my uniforms.
Suspicious indeed... what is the correct way to map, then, without
allocating a uniform for samplers and other not-real-uniform-uniforms?
The hardware just wants a vec4 index; NIR mirrors the GLSL; poof?
I think I had troubles there, but I can't recall exactly.
> Using info.outputs_written might be nicer here.
Mayhaps... I have to transform order anyway, or establish a generic
interface for communicating order back to the cmdstream bits and resolve
it dynamically there. OTOH, maybe that's the right way to go anyway;
a lot of this code grew "organically" and the details of varying
descriptors were only understood recently, long after the first batch of
that was written... I suppose this could be a good refactor.
> I'm skeptical that this many lower_var_copies() is needed :)
^_^
I gotta make sure they're _really_ lowered! ;)
> I need to steal your isign.
Bon apetit.
> > + (('fge', a, b), ('flt', b, a)),
> > +
> > + # XXX: We have hw ops for this, just unknown atm..
> > + #(('fsign@32', a), ('i2f32@32', ('isign', ('f2i32@32', ('fmul', a,
> > 0x43800000)))))
> > + #(('fsign', a), ('fcsel', ('fge', a, 0), 1.0, ('fcsel', ('flt', a,
> > 0.0), -1.0, 0.0)))
> > + (('fsign', a), ('bcsel', ('fge', a, 0), 1.0, -1.0)),
>
> Looks like your fsign never returns 0.0 like it should?
Indeed it does not. I should maybe figure out what "hw ops" I was
referring to; less risk of bugs that way, I suppose.
> All of this is suggestions for future work. I'm mostly glad to see the
> driver coming into the tree at last. Both patches are:
>
> Acked-by: Eric Anholt <e...@anholt.net>
Thank you! As I mentioned in the other email (to Rob), is there anything
particular blocking a push into master?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
