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

[Ilia Mirkin]

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.

Cheers,

  -ilia