[Mesa-dev] [PATCH] i965: do not release GLSL IR for SSO programs

[Tapani Pälli]

On 10/26/2016 11:21 AM, Timothy Arceri wrote:
> On Wed, 2016-10-26 at 08:50 +0300, Tapani Pälli wrote:
>>
>> On 10/26/2016 08:15 AM, Timothy Arceri wrote:
>>>
>>> On Tue, 2016-10-25 at 09:39 +0300, Tapani Pälli wrote:
>>>>
>>>> SSO shader programs can be later modified by attaching/detaching
>>>> shaders and relinked, this requires IR.
>>>
>>> Doesn't relinking recreate the IR? We can relink exiting shaders
>>> into
>>> new programs. The IR is cloned from gl_shader (the compiled IR)
>>> before
>>> this happens.
>>>
>>> Where exactly are things falling over for SSO?
>>
>> I went this way as I haven't seen this happening elsewhere but when
>> relinking program created by glCreateShaderProgram. TBH I'm not sure
>> if
>> this could happen with regular programs, I would assume we had
>> already
>> bugs if it would be so (?)
>>
>> In practice things go bad in brw_link_shader where process_glsl_ir()
>> gets called, like this:
>>
>> --- 8< ---
>> #0  do_expression_flattening (instructions=instructions at entry=0x0,
>> predicate=predicate at entry=0x7ffff0f808a0
>> <mat_op_to_vec_predicate(ir_instruction*)>) at
>> glsl/ir_expression_flattening.cpp:60
>> #1  0x00007ffff0f816b9 in do_mat_op_to_vec (instructions=0x0) at
>> glsl/lower_mat_op_to_vec.cpp:96
>> #2  0x00007ffff10385bd in process_glsl_ir (shader_prog=0x9b74d8,
>> shader=0x9b2688, brw=0x7d2478) at brw_link.cpp:108
>> #3  brw_link_shader (ctx=0x7d2478, shProg=0x9b74d8) at
>> brw_link.cpp:234
>> #4  0x00007ffff0ec1f31 in _mesa_glsl_link_shader (ctx=0x7d2478,
>> prog=0x9b74d8) at program/ir_to_mesa.cpp:3067
>> #5  0x00007ffff0ddc99b in _mesa_link_program (ctx=0x7d2478,
>> shProg=0x9b74d8) at main/shaderapi.c:1098
>> #6  0x00007ffff7ac8d15 in stub_glLinkProgram (program=2) at
>> /home/tpalli/source/fdo/piglit/tests/util/piglit-dispatch-gen.c:33005
>> #7  0x00000000004019ab in
>> relink_program_created_by_glCreateShaderProgram () at
>> /home/tpalli/source/fdo/piglit/tests/spec/arb_separate_shader_objects
>> /api-errors.c:78
>
> There is a comment above that line in the test.
>
> /* Issue #14 of the GL_ARB_separate_shader_objects spec says:
> *
>  *     "14. Should glLinkProgram work to re-link a shader created with
>  *          glCreateShaderProgram?
>  *
>  *          RESOLVED: NO because the shader created by
>  *          glCreateShaderProgram is detached and deleted as part of
>  *          the glCreateShaderProgram sequence.  This means if you
>  *          call glLinkProgram on a program returned from
>  *          glCreateShaderProgram, you'll find the re-link fails
>  *          because no shader object is attached.
>  *
>  *          An application is free to attach one or more new shader
>  *          objects to the program and then relink would work.
>  *
>  *          This is fine because re-linking isn't necessary/expected."
>  */
>
> Which means we shouldn't able to relink this shader. We shouldn't be
> able to get as far along as we do when we get the error message.

Ah right, so this should be detected somewhere within shaderapi, will 
take a look there. The reason I suspected we end here with SSO was that 
this fails just on the old gen models so something definitely goes 
different ways with those. But I'll look at shaderapi first.



>>
>>
>>>
>>>>
>>>>  This patch fixes regression
>>>> caused by 4542c7ed5fc6d8cb2495d322b4f06d802d7292cc.
>>>>
>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
>>>> Cc: "12.0 13.0" <mesa-stable at lists.freedesktop.org>
>>>> ---
>>>>  src/mesa/drivers/dri/i965/brw_link.cpp | 17 ++++++++++-------
>>>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp
>>>> b/src/mesa/drivers/dri/i965/brw_link.cpp
>>>> index 5ea9773..ffb66a9 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_link.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_link.cpp
>>>> @@ -290,14 +290,17 @@ brw_link_shader(struct gl_context *ctx,
>>>> struct
>>>> gl_shader_program *shProg)
>>>>
>>>>     build_program_resource_list(ctx, shProg);
>>>>
>>>> -   for (stage = 0; stage < ARRAY_SIZE(shProg->_LinkedShaders);
>>>> stage++) {
>>>> -      struct gl_linked_shader *shader = shProg-
>>>>>
>>>>> _LinkedShaders[stage];
>>>> -      if (!shader)
>>>> -         continue;
>>>> +   /* We can't free IR for SSO programs since those may need
>>>> relinking. */
>>>> +   if (!shProg->SeparateShader) {
>>>> +      for (stage = 0; stage < ARRAY_SIZE(shProg-
>>>>> _LinkedShaders);
>>>> stage++) {
>>>> +         struct gl_linked_shader *shader = shProg-
>>>>>
>>>>> _LinkedShaders[stage];
>>>> +         if (!shader)
>>>> +            continue;
>>>>
>>>> -      /* The GLSL IR won't be needed anymore. */
>>>> -      ralloc_free(shader->ir);
>>>> -      shader->ir = NULL;
>>>> +         /* The GLSL IR won't be needed anymore. */
>>>> +         ralloc_free(shader->ir);
>>>> +         shader->ir = NULL;
>>>> +      }
>>>>     }
>>>>
>>>>     return true;
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev