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

[Ian Romanick]

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?

>> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
>>> ---
>>>  src/compiler/glsl/linker.cpp | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>>> index e712ee3..010dbd7 100644
>>> --- a/src/compiler/glsl/linker.cpp
>>> +++ b/src/compiler/glsl/linker.cpp
>>> @@ -3769,9 +3769,9 @@ add_shader_variable(struct gl_shader_program *shProg, unsigned stage_mask,
>>>         *    the name of the interface block (not the instance name) and
>>>         *    "Member" is the name of the variable."
>>>         */
>>> -      const char *prefixed_name = (var->data.from_named_ifc_block &&
>>> -                                   !is_gl_identifier(var->name))
>>> -         ? ralloc_asprintf(shProg, "%s.%s", var->get_interface_type()->name,
>>> +      const char *prefixed_name = var->data.from_named_ifc_block
>>> +         ? ralloc_asprintf(shProg, "%s.%s",
>>> +                           var->get_interface_type()->without_array()->name,
>>>                             name)
>>>           : name;
>>>
>>> --
>>> 2.5.5
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev