[Mesa-dev] [PATCH 2/2] panfrost: Implement Midgard shader toolchain

[Alyssa Rosenzweig]

> 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 at 32', a), ('i2f32 at 32', ('isign', ('f2i32 at 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 <eric at anholt.net>

Thank you! As I mentioned in the other email (to Rob), is there anything
particular blocking a push into master?