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

[Ilia Mirkin]

On Wed, Oct 26, 2016 at 8:08 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> On Wed, 2016-10-26 at 22:51 +1100, Timothy Arceri wrote:
>> On Wed, 2016-10-26 at 13:13 +0300, Tapani Pälli wrote:
>> >
>> > Hi;
>> >
>> > On 10/26/2016 11:27 AM, Tapani Pälli wrote:
>> > >
>> > >
>> > >
>> > >
>> > > 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
>> > > > > =0
>> > > > > x0,
>> > > > > 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
>> > > > > _o
>> > > > > bjects
>> > > > > /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.
>> >
>> > Now I realized that I could bail in linker with following:
>> > if (shProg->SeparateShader && shProg->NumShaders == 0)
>> >
>> > BUT following commit states that having NumShaders == 0 is OK for
>> > compatibility profile:
>> >
>> > 76cfb472077dc83c892b4cddf79333341deaa7b5
>> >
>> > Timothy, do you think I could still bailout with the condition
>> > above?
>> > I
>> > really do wonder the case mentioned in commit where someone links
>> > a
>> > shader without any stages, does this really make sense (what is the
>> > use
>> > case) or is this allowed just because spec does not happen to
>> > prohibit this?
>>
>> ok, I see. The test says it *should* relink when in compat profile.
>
> Actually we still return early it just shouldn't throw an error.
>
>> In
>> which case it should fall back to fixed-function. I'm not entirely
>> sure
>> what path things take from here but it seems something is not working
>> correctly when creating the fallback shader.
>
> I've taken a closer look. We don't do anything about creating fallback
> shaders in GLSL IR so we shouldn't be hitting the code where the error
> happens.
>
> I believe the problem is in glDetachShader() we remove the gl_shader
> but we leave gl_linked_shader in place.

AFAIK that's a valid sequence - glAttachShader(); glLinkProgram();
glDetachShader(); glDeleteShader(); - the program should still be
usable.

  -ilia