[Mesa-dev] [Mesa-stable] [PATCH v2 03/11] mesa: Strip arrayness from interface block names in some IO validation

[Ilia Mirkin]

On Thu, Jun 23, 2016 at 6:07 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 06/16/2016 12:12 PM, Ilia Mirkin wrote:
>> On Thu, Jun 16, 2016 at 3:06 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> Outputs from the vertex shader need to be able to match
>>> per-vertex-arrayed inputs of later stages.  Acomplish this by stripping
>>> one level of arrayness from the names and types of outputs going to a
>>> per-vertex-arrayed stage.
>>>
>>> v2: Add missing checks for TESS_EVAL->GEOMETRY.  Noticed by Timothy
>>> Arceri.
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96358
>>> Cc: "12.0" <mesa-stable at lists.freedesktop.org>
>>> Cc: Gregory Hainaut <gregory.hainaut at gmail.com>
>>> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
>>> ---
>>>  src/mesa/main/shader_query.cpp | 100 +++++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 92 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp
>>> index 5956ce4..a6a0a2f 100644
>>> --- a/src/mesa/main/shader_query.cpp
>>> +++ b/src/mesa/main/shader_query.cpp
>>> @@ -1385,13 +1385,26 @@ _mesa_get_program_resourceiv(struct gl_shader_program *shProg,
>>>
>>>  static bool
>>>  validate_io(struct gl_shader_program *producer,
>>> -            struct gl_shader_program *consumer)
>>> +            struct gl_shader_program *consumer,
>>> +            gl_shader_stage producer_stage,
>>> +            gl_shader_stage consumer_stage)
>>>  {
>>>     if (producer == consumer)
>>>        return true;
>>>
>>> +   const bool nonarray_stage_to_array_stage =
>>> +      (producer_stage == MESA_SHADER_VERTEX &&
>>> +       (consumer_stage == MESA_SHADER_GEOMETRY ||
>>> +        consumer_stage == MESA_SHADER_TESS_CTRL ||
>>> +        consumer_stage == MESA_SHADER_TESS_EVAL)) ||
>>> +      (producer_stage == MESA_SHADER_TESS_EVAL &&
>>> +       consumer_stage == MESA_SHADER_GEOMETRY);
>>
>> Since TCS -> GEOM is not possible, isn't the only way that array ->
>> array can happen is TCS -> TES? IOW, couldn't this just be
>>
>> producer != TCS && (consumer == TCS || consumer == TES || consumer == GS)
>>
>> Your call whether you want to rewrite it. [No comment on the rest of
>> the patch...]
>
> I had though of another (slightly less) clever way to do this, but I
> decided against it because it was less clear.
>
> Anyway, I tried your suggestion.  Here's the assembly 'diff
> --side-by-side'.  Mine is on the left, yours is on the right.
>
>     mov    0x4(%rax),%eax                mov    0x4(%rax),%eax
>     cmp    %rbp,%rbx                     cmp    %rbp,%rbx
>     je     374c                     |    je     3744
>     test   %eax,%eax                |    movb   $0x0,0x1f(%rsp)
>     jne    357b                     |    cmp    $0x1,%eax
>     lea    -0x1(%rdx),%ecx          |    je     357f
>     movb   $0x1,0x1f(%rsp)          |    sub    $0x1,%edx
>     cmp    $0x2,%ecx                |    cmp    $0x2,%edx
>     jbe    358d                     |    setbe  0x1f(%rsp)
>     cmp    $0x2,%eax                <
>     sete   %cl                      <
>     cmp    $0x3,%edx                <
>     sete   %al                      <
>     and    %eax,%ecx                <
>     mov    %cl,0x1f(%rsp)           <
>
> I don't know that we'd be able to measure a performance difference.
> Maybe having one less branch that may not predict well is good?  *shrug*

A little intense that you looked at the asm... my argument was mostly
for the sake of simplicity and readability. If you don't think my
version is easier to understand, then keep yours.

  -ilia