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

[Ian Romanick]

On 06/23/2016 03:30 PM, Ilia Mirkin wrote:
> 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.

Heh... I just thought it was due diligence. :)

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