[Mesa-dev] [PATCH 3/3] glsl/linker: validate explicit locations for SSO programs
tarceri at itsqueeze.com
Fri Oct 20 15:55:02 UTC 2017
On 21/10/17 00:35, Ilia Mirkin wrote:
> On Fri, Oct 20, 2017 at 6:46 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
>> src/compiler/glsl/link_varyings.cpp | 53 +++++++++++++++++++++++++++++++++++++
>> src/compiler/glsl/link_varyings.h | 4 +++
>> src/compiler/glsl/linker.cpp | 6 +++++
>> 3 files changed, 63 insertions(+)
>> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
>> index dffb4f98df..d7e407184d 100644
>> --- a/src/compiler/glsl/link_varyings.cpp
>> +++ b/src/compiler/glsl/link_varyings.cpp
>> @@ -599,6 +599,59 @@ validate_explicit_variable_location(struct gl_context *ctx,
>> + * Validate explicit locations for SSO programs. For non-SSO programs this
>> + * is alreadty done in cross_validate_outputs_to_inputs().
>> + */
>> +validate_sso_explicit_locations(struct gl_context *ctx,
>> + struct gl_shader_program *prog)
>> + assert(prog->SeparateShader);
>> + struct explicit_location_info explicit_locations_in[MAX_VARYINGS_INCL_PATCH];
>> + struct explicit_location_info explicit_locations_out[MAX_VARYINGS_INCL_PATCH];
> These comments may point to issues in earlier patches / overall logic,
> but I'll still make them here. I don't have time to do a full review
> of all the patches, unfortunately. Thanks for addressing my concerns
> with your earlier patchset.
> This should be MAX_VARYINGS. The patch varyings are meant to be
> aliased against the non-patch varyings, and their indices must be
> assigned as such, otherwise you won't get the overlaps you're supposed
Are you sure this is really required?
> Furthermore, I haven't checked how your code works, but I found it was
> easier to not have the . None of the checks need to be
> per-component (and in fact, you're supposed to raise an error when
> components disagree on type-related things, except for VS inputs which
> you skip here anyways). Just store the first value in the info, and
> then if anything comes in counter to that, return an error. You can
> see what I did in https://patchwork.freedesktop.org/patch/175959/. I
> spent quite some time to make sure that the patch was correct, so if
> you see any functional differences to what you've done, I'd recommend
> considering why there are differences -- there shouldn't be any.
As I keep pointing out your patch doesn't handle component aliasing.
Dropping the  would mean we not longer check for this.
More information about the mesa-dev