[Mesa-dev] [PATCH v2 2/8] glsl/linker: refactor link-time validation of output locations
Iago Toral
itoral at igalia.com
Wed Oct 25 09:21:39 UTC 2017
On Tue, 2017-10-24 at 23:40 -0400, Ilia Mirkin wrote:
> On Tue, Oct 24, 2017 at 5:28 AM, Iago Toral Quiroga <itoral at igalia.co
> m> wrote:
> > Move the checks for explicit locations to a separate function. We
> > will use this in a follow-up patch to validate locations for
> > interface
> > variables where we need to validate each interface member rather
> > than
> > the interface variable itself.
> >
> > Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
> > ---
> > src/compiler/glsl/link_varyings.cpp | 128 ++++++++++++++++++++--
> > --------------
> > 1 file changed, 73 insertions(+), 55 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index cb9091d86b..d394488ab9 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -403,6 +403,75 @@ compute_variable_location_slot(ir_variable
> > *var, gl_shader_stage stage)
> > return var->data.location - location_start;
> > }
> >
> > +static bool
> > +check_location_aliasing(ir_variable *explicit_locations[][4],
>
> It's also checking component aliasing. Maybe just check_aliasing() ?
check_aliasing() sounds too generic to me since it doesn't indicate
anything about what is the target of the aliasing checks, so I'd prefer
to keep it as is unless you have strong opinion about changing it.
> > + ir_variable *var,
> > + unsigned location,
> > + unsigned component,
> > + unsigned location_limit,
> > + const glsl_type *type,
> > + gl_shader_program *prog,
> > + gl_shader_stage stage)
> > +{
> > + unsigned last_comp;
> > + if (type->without_array()->is_record()) {
> > + /* The component qualifier can't be used on structs so just
> > treat
> > + * all component slots as used.
> > + */
> > + last_comp = 4;
> > + } else {
> > + unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > + last_comp = component + type->without_array()-
> > >vector_elements * dmul;
> > + }
> > +
> > + while (location < location_limit) {
> > + unsigned i = component;
> > + while (i < last_comp) {
> > + if (explicit_locations[location][i] != NULL) {
> > + linker_error(prog,
> > + "%s shader has multiple outputs
> > explicitly "
> > + "assigned to location %d and component
> > %d\n",
> > + _mesa_shader_stage_to_string(stage),
> > + location, component);
> > + return false;
> > + }
> > +
> > + /* Make sure all component at this location have the same
> > type.
> > + */
> > + for (unsigned j = 0; j < 4; j++) {
> > + if (explicit_locations[location][j] &&
> > + (explicit_locations[location][j]->type-
> > >without_array()
> > + ->base_type != type->without_array()->base_type))
> > {
>
> I believe this is too strict. e.g. one is allowed to have int and
> uint
> be shared in the same slot, even though they'll have different base
> types.
Yes, you're right.
> Also you appear to be doing this for each and every variable, which
> is
> inefficient. This check should just be done once at the end. (Perhaps
> there's a later patch which fixes these concerns. If that's the case,
> then this one is fine as it's just moving code around.)
>
> > + linker_error(prog,
> > + "Varyings sharing the same location
> > must "
> > + "have the same underlying numerical
> > type. "
> > + "Location %u component %u\n",
> > location, component);
> > + return false;
> > + }
> > + }
> > +
> > + explicit_locations[location][i] = var;
> > + i++;
> > +
> > + /* We need to do some special handling for doubles as
> > dvec3 and
> > + * dvec4 consume two consecutive locations. We don't need
> > to
> > + * worry about components beginning at anything other
> > than 0 as
> > + * the spec does not allow this for dvec3 and dvec4.
> > + */
> > + if (i == 4 && last_comp > 4) {
> > + last_comp = last_comp - 4;
> > + /* Bump location index and reset the component index
> > */
> > + location++;
> > + i = 0;
> > + }
> > + }
> > +
> > + location++;
> > + }
> > +
> > + return true;
> > +}
> > +
> > /**
> > * Validate that outputs from one stage match inputs of another
> > */
> > @@ -435,7 +504,6 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> > unsigned num_elements = type-
> > >count_attribute_slots(false);
> > unsigned idx = compute_variable_location_slot(var,
> > producer->Stage);
> > unsigned slot_limit = idx + num_elements;
> > - unsigned last_comp;
> >
> > unsigned slot_max =
> > ctx->Const.Program[producer-
> > >Stage].MaxOutputComponents / 4;
> > @@ -446,60 +514,10 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> > return;
> > }
> >
> > - if (type->without_array()->is_record()) {
> > - /* The component qualifier can't be used on structs so
> > just treat
> > - * all component slots as used.
> > - */
> > - last_comp = 4;
> > - } else {
> > - unsigned dmul = type->without_array()->is_64bit() ? 2
> > : 1;
> > - last_comp = var->data.location_frac +
> > - type->without_array()->vector_elements * dmul;
> > - }
> > -
> > - while (idx < slot_limit) {
> > - unsigned i = var->data.location_frac;
> > - while (i < last_comp) {
> > - if (explicit_locations[idx][i] != NULL) {
> > - linker_error(prog,
> > - "%s shader has multiple outputs
> > explicitly "
> > - "assigned to location %d and
> > component %d\n",
> > - _mesa_shader_stage_to_string(produc
> > er->Stage),
> > - idx, var->data.location_frac);
> > - return;
> > - }
> > -
> > - /* Make sure all component at this location have
> > the same type.
> > - */
> > - for (unsigned j = 0; j < 4; j++) {
> > - if (explicit_locations[idx][j] &&
> > - (explicit_locations[idx][j]->type-
> > >without_array()
> > - ->base_type != type->without_array()-
> > >base_type)) {
> > - linker_error(prog,
> > - "Varyings sharing the same
> > location must "
> > - "have the same underlying
> > numerical type. "
> > - "Location %u component %u\n",
> > idx,
> > - var->data.location_frac);
> > - return;
> > - }
> > - }
> > -
> > - explicit_locations[idx][i] = var;
> > - i++;
> > -
> > - /* We need to do some special handling for doubles
> > as dvec3 and
> > - * dvec4 consume two consecutive locations. We
> > don't need to
> > - * worry about components beginning at anything
> > other than 0 as
> > - * the spec does not allow this for dvec3 and
> > dvec4.
> > - */
> > - if (i == 4 && last_comp > 4) {
> > - last_comp = last_comp - 4;
> > - /* Bump location index and reset the component
> > index */
> > - idx++;
> > - i = 0;
> > - }
> > - }
> > - idx++;
> > + if (!check_location_aliasing(explicit_locations, var,
> > idx,
> > + var->data.location_frac,
> > slot_limit,
> > + type, prog, producer-
> > >Stage)) {
> > + return;
> > }
> > }
> > }
> > --
> > 2.11.0
> >
>
>
More information about the mesa-dev
mailing list