[Mesa-dev] [PATCH 02/58] mesa: optimise interleaved sso validation

[Ian Romanick]

On 11/28/2016 09:12 PM, Timothy Arceri wrote:
> On Mon, 2016-11-28 at 18:59 -0800, Ian Romanick wrote:
>> On 11/20/2016 05:28 AM, Timothy Arceri wrote:
>>>
>>> Now that we have a linked_stages bitfield we can use this
>>> to check if the program is used at a later stage.
>>>
>>> This change is also required to be able to use gl_program
>>> rather than gl_shader_program in the CurrentProgram array.
>>> ---
>>>  src/mesa/main/pipelineobj.c | 17 +++++++----------
>>>  1 file changed, 7 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/src/mesa/main/pipelineobj.c
>>> b/src/mesa/main/pipelineobj.c
>>> index 35d416d..45131d2 100644
>>> --- a/src/mesa/main/pipelineobj.c
>>> +++ b/src/mesa/main/pipelineobj.c
>>> @@ -730,30 +730,27 @@ program_stages_all_active(struct
>>> gl_pipeline_object *pipe,
>>>  static bool
>>>  program_stages_interleaved_illegally(const struct
>>> gl_pipeline_object *pipe)
>>>  {
>>> -   struct gl_shader_program *prev = NULL;
>>> -   unsigned i, j;
>>> +   unsigned prev_linked_stages = 0;
>>>  
>>>     /* Look for programs bound to stages: A -> B -> A, with any
>>> intervening
>>>      * sequence of unrelated programs or empty stages.
>>>      */
>>> -   for (i = 0; i < MESA_SHADER_STAGES; i++) {
>>> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>>>        struct gl_shader_program *cur = pipe->CurrentProgram[i];
>>>  
>>>        /* Empty stages anywhere in the pipe are OK */
>>> -      if (!cur || cur == prev)
>>> +      if (!cur || cur->data->linked_stages == prev_linked_stages)
>>
>> The previous 'cur == prev' check would prevent validation when, say,
>> a
>> program with a linked vertex shader and geometry shader were attached
>> to
>> a pipeline.  Just looking at this line of code, it's not immediately
>> obvious why the new check is equivalent.  I'm guessing that code
>> elsewhere generates an error if the application binds a programs with
>> the same set of stages to a pipeline
>>
>>     glGenProgramPipelines(1, &pipe);
>>     glGenPrograms(2, prog);
>>
>>     // compile and link two programs that have a vertex & fragment
>>     // shader.
>>     // ...
>>
>>     glBindProgramPipeline(pipe);
>>     glUseProgramStages(pipe, GL_VERTEX_SHADER_BIT, prog[0]);
>>     glUseProgramStages(pipe, GL_FRAGMENT_SHADER_BIT, prog[1]);
>>
>> In the existing code, this pipeline will get validated here (and
>> fail).
>> With the new code, it will not get validated here.
> 
> Your guess was correct. The check above this
> one program_stages_all_active() ensures that all stages of a program
> are active so we will always fail before validating interleaved stages
> in the example you gave. Therefore cur->data->linked_stages ==
> prev_linked_stages can only ever mean that we are looking at the same
> program.
> 
> I could add a comment if you think it would help?

Yeah.  With that added, patches 1 and 2 are

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

>>>
>>>           continue;
>>>  
>>> -      if (prev) {
>>> +      if (prev_linked_stages) {
>>>           /* We've seen an A -> B transition; look at the rest of
>>> the pipe
>>>            * to see if we ever see A again.
>>>            */
>>> -         for (j = i + 1; j < MESA_SHADER_STAGES; j++) {
>>> -            if (pipe->CurrentProgram[j] == prev)
>>> -               return true;
>>> -         }
>>> +         if (prev_linked_stages >> (i + 1))
>>> +            return true;
>>>        }
>>>  
>>> -      prev = cur;
>>> +      prev_linked_stages = cur->data->linked_stages;
>>>     }
>>>  
>>>     return false;
>>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev