[Mesa-dev] [PATCH 05/10] glsl: Add all system variables to the input resource list.

[Kenneth Graunke]

On Thursday, March 31, 2016 3:00:37 PM PDT Ilia Mirkin wrote:
> On Thu, Mar 31, 2016 at 2:53 PM, Kenneth Graunke <kenneth at whitecape.org> 
wrote:
> > System values are just built-in input variables that we've opted to
> > special-case out of convenience.  We need to consider all inputs,
> > regardless of how we've classified them.
> >
> > Unfortunately, there's one exception: we shouldn't add gl_BaseVertex
> > unless ARB_shader_draw_parameters is enabled, because it doesn't
> > actually exist in the language, and shouldn't be counted in the
> > GL_ACTIVE_RESOURCES query.
> >
> > Fixes dEQP-GLES31.functional.program_interface_query.program_input.
> > resource_list.compute.empty, which expects gl_NumWorkGroups to appear
> > in the resource list.
> >
> > v2: Delete more code
> > v3: Add basevertex hack back in to avoid regressing CTS tests.
> >
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/compiler/glsl/linker.cpp   | 15 +++++++--------
> >  src/mesa/main/shader_query.cpp |  9 +--------
> >  2 files changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> > index 29c63ee..48aa395 100644
> > --- a/src/compiler/glsl/linker.cpp
> > +++ b/src/compiler/glsl/linker.cpp
> > @@ -3531,15 +3531,14 @@ add_interface_variables(struct gl_shader_program 
*shProg,
> >           continue;
> >
> >        switch (var->data.mode) {
> > -      /* From GL 4.3 core spec, section 11.1.1 (Vertex Attributes):
> > -       * "For GetActiveAttrib, all active vertex shader input variables
> > -       * are enumerated, including the special built-in inputs 
gl_VertexID
> > -       * and gl_InstanceID."
> > -       */
> >        case ir_var_system_value:
> > -         if (var->data.location != SYSTEM_VALUE_VERTEX_ID &&
> > -             var->data.location != SYSTEM_VALUE_VERTEX_ID_ZERO_BASE &&
> > -             var->data.location != SYSTEM_VALUE_INSTANCE_ID)
> > +         /* We use gl_BaseVertex internally for gl_VertexID lowering.
> > +          * We don't want it to leak into the program resource list.
> > +          *
> > +          * TODO: We do need to add it when ARB_shader_draw_parameters
> > +          *       is in use.  We've lost that information by now...
> > +          */
> > +         if (var->data.location == SYSTEM_VALUE_BASE_VERTEX)
> 
> Perhaps only do this if the lowering is enabled? That way this will
> only be an issue for i965. i.e.
> 
> if (ctx->Const.VertexID_is_zero_based && loc == BASE_VERTEX)
> 
> I guess it's a little annoying since you don't get the context in this
> function :( Perhaps there's some clever way of getting at it? If not,
> then never mind =/

I came up with a better approach:

https://lists.freedesktop.org/archives/mesa-dev/2016-April/111625.html
https://lists.freedesktop.org/archives/mesa-dev/2016-April/111626.html

With those patches in place (before this series), then we can delete
this hunk entirely, so it's just a straight fallthrough:

       case ir_var_system_value:
       case ir_var_shader_in:

> 
> >              continue;
> >           /* FALLTHROUGH */
> >        case ir_var_shader_in:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160401/49fa5fd6/attachment-0001.sig>