On 12 December 2012 14:49, Eric Anholt <e...@anholt.net> wrote:
> Paul Berry <stereotype...@gmail.com> writes:
> > diff --git a/src/glsl/lower_packed_varyings.cpp
> b/src/glsl/lower_packed_varyings.cpp
> > new file mode 100644
> > index 0000000..4cb9066
> > --- /dev/null
> > +++ b/src/glsl/lower_packed_varyings.cpp
> > @@ -0,0 +1,371 @@
> > +/*
> > + * Copyright © 2011 Intel Corporation
>
> 2012
>
> > +/**
> > + * \file lower_varyings_to_packed.cpp
>
> lower_packed_varyings.cpp
>
> > + /**
> > + * Number of generic varying slots which are used by this shader.
> This is
> > + * used to allocate temporary intermediate data structures. If any
> any
>
> Too many "any"s.
>
> > + * varying used by this shader has a location greater than or equal
> to
> > + * location_base + locations_used, an assertion will fire.
> > + */
> > + const unsigned locations_used;
>
>
> > +ir_visitor_status
> > +lower_packed_varyings_visitor::visit(ir_variable *var)
> > +{
> > + if (var->mode != this->mode ||
> > + var->location < (int) this->location_base ||
> > + !this->needs_lowering(var))
> > + return visit_continue;
> > +
> > + /* Change the old varying into an ordinary global. */
> > + var->mode = ir_var_auto;
> > +
> > + /* Create a reference to the old varying. */
> > + ir_dereference_variable *deref
> > + = new(this->mem_ctx) ir_dereference_variable(var);
> > +
> > + /* Recursively pack or unpack it. */
> > + this->lower_rvalue(deref, var->location * 4 + var->location_frac,
> var);
> > +
> > + return visit_continue;
> > +}
>
> I think an alternative here, instead of using the visitor class and
> skipping functions, would be to just walk the top-level ir_instruction
> list, check if it's a variable, and the run this function body on it.
>
Yeah, I considered that too. I don't really have a strong preference
either way.
>
> > +unsigned
> > +lower_packed_varyings_visitor::lower_rvalue(ir_rvalue *rvalue,
> > + unsigned fine_location,
> > + ir_variable *unpacked_var)
> > +{
>
> I will admit I didn't totally follow the path of the swizzles through
> this code, but that's my fault and I expect testing to have covered it.
>
> > + unsigned slot = location - this->location_base;
> > + assert(slot < locations_used);
> > + if (this->packed_varyings[slot] == NULL) {
> > + char name[10];
> > + sprintf(name, "packed%d", slot);
>
> Bonus points if this could also strcat the variable names contributing
> to it somehow. Turning all my FS inputs into packedN is ugly for shader
> debugging.
>
Yeah, I can see why that would be useful. I'll spend a little time on it
and see if I can do it without complicating the code too much--if I can't,
I'll leave the bonus points available for someone else to pick up :)
>
> > + const glsl_type *packed_type;
> > + switch (unpacked_var->type->get_scalar_type()->base_type) {
> > + case GLSL_TYPE_UINT:
> > + packed_type = glsl_type::uvec4_type;
> > + break;
> > + case GLSL_TYPE_INT:
> > + packed_type = glsl_type::ivec4_type;
> > + break;
> > + case GLSL_TYPE_FLOAT:
> > + packed_type = glsl_type::vec4_type;
> > + break;
> > + case GLSL_TYPE_BOOL:
> > + /* Note: in theory bools aren't supposed to be allowed to be
> > + * varyings, however Mesa doesn't know to reject them yet.
> Until
> > + * this is fixed, we might as well allow them to be packed too.
> > + */
> > + packed_type = glsl_type::bvec4_type;
> > + break;
> > + default:
> > + assert(!"Unexpected varying type while packing");
> > + packed_type = glsl_type::vec4_type;
> > + break;
> > + }
>
> Maybe just:
>
> packed_type = get_instance(unpacked_var->get_scalar_type()->base_type, 4,
> 1);
>
Aha, yes! That's way better.
>
> At this point I've read through the series, and only had trivial
> review/suggestions. Fix what you like and:
>
> Reviewed-by: Eric Anholt <e...@anholt.net>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
