[Mesa-dev] [Mesa-stable] [PATCH 2/5] glsl/linker: Properly report gl_PerVertex shader inputs

[Timothy Arceri]

On Tue, 2016-05-31 at 17:26 -0400, Ilia Mirkin wrote:
> On Tue, May 31, 2016 at 5:09 PM, Ian Romanick <idr at freedesktop.org>
> wrote:
> > 
> > On 05/31/2016 01:48 PM, Ian Romanick wrote:
> > > 
> > > On 05/31/2016 11:56 AM, Ilia Mirkin wrote:
> > > > 
> > > > On Tue, May 31, 2016 at 2:52 PM, Ian Romanick <idr at freedesktop.
> > > > org> wrote:
> > > > > 
> > > > > From: Ian Romanick <ian.d.romanick at intel.com>
> > > > > 
> > > > > Issue #16 of the GL_ARB_program_interface_query makes it
> > > > > pretty clear
> > > > > array shader inputs of gl_PerVertex blocks should be reported
> > > > > as
> > > > > gl_PerVertex.gl_Foo.  Piglit tests were recently changed to
> > > > > expect
> > > > > this behavior, and this change makes those tests pass again.
> > > > I'd like to note that you're now removing arrays of *all*
> > > > interfaces,
> > > > not just gl_PerVertex. I suspect this is OK, but just want to
> > > > double-check. Assuming that was your intent,
> > > It was the intent when I wrote it, but I wasn't sure whether or
> > > not it
> > > was correct.  I ran it through piglit, CTS, and dEQP, and there
> > > were no
> > > changes.  I was intending to go dig deeper in the spec, but I
> > > forgot.
> > > I'll double check that.
> > Right... so here's the weird thing.  I feel like it's all
> > junk.  The
> > ARB_piq spec clearly says:
> > 
> >       * For an active interface block declared as an array of
> > instances,
> >         separate entries will be generated for each active
> > instance.  The name
> >         of the instance is formed by concatenating the block name,
> > the "["
> >         character, an integer identifying the instance number, and
> > the "]"
> >         character.
> > 
> > By my reading, even gl_PerVertex stuff should come through as
> > gl_PerVertex[0].gl_Foo through gl_PerVertex[#].gl_Foo.  If that's
> > true,
> > then even the dEQP tests are bunk.
> > 
> > Before we would return BlockName[#array_size].foo, and that is
> > definitely wrong.  Now we'll return BlockName.foo.  This possibly
> > not
> > right, but I don't think it's worse than what we did before... but
> > we
> > now do what dEQP expects for gl_PerVertex.
> > 
> > I think I want to push this patch with some of the commentary above
> > added to the commit message.  How does that sound?
> Looking closer, I think you're right - issue 16 says:
> 
>     (16) How are inputs and outputs in the built-in interface block
>          "gl_PerVertex" enumerated?
> 
>       RESOLVED:  We will follow the default rule for enumerating
> block members
>       in the OpenGL API, which is:
> 
>         * If a variable is a member of an interface block without an
> instance
>           name, it is enumerated using just the variable name.
> 
>         * If a variable is a member of an interface block with an
> instance
>           name, it is enumerated as "BlockName.Member", where
> "BlockName" is
>           the name of the interface block (not the instance name) and
> "Member"
>           is the name of the variable.
> 
>       For example, in the following code:
> 
>         uniform Block1 {
>           int member1;
>         };
>         uniform Block2 {
>           int member2;
>         } instance2;
>         uniform Block3 {
>           int member3;
>         } instance3[2];  // uses two separate buffer bindings
> 
>       the three uniforms (if active) are enumerated as "member1",
>       "Block2.member2", and "Block3.member3".
> 
> However the ACTUAL rules for enumerating block members are:
> 
>       * For an active interface block not declared as an array of
> block
>         instances, a single entry will be generated, using the block
> name from
>         the shader source.
> 
>       * For an active interface block declared as an array of
> instances,
>         separate entries will be generated for each active
> instance.  The name
>         of the instance is formed by concatenating the block name,
> the "["
>         character, an integer identifying the instance number, and
> the "]"
>         character.
> 
> So I think in that example, it should be Block1.member1, and
> Block3[0].member3 + Block3[1].member3?
> 
> Anyways, this is all very confusing to me. I'd like to withdraw my
> R-b, since I clearly don't understand what's going on. Hopefully a
> more experienced spec reader will be able to do a real review.

Yeah this seems like an oversight to me. I was looking at this the
other day and it seemed to me we should be using or at least doing
something like how the names are generated in the
program_resource_visitor in link_uniforms.cpp which is used for both
UBO's and interface blocks.

I also added create_xfb_varying_names() a while back too which does a
similar thing.

The code in add_shader_variable() that deals with structs to me also
looks like it should be taking into account arrays.

> 
> Cheers,
> 
>   -ilia
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-stable