[Mesa-dev] [PATCH] glsl/linker: Fix out variables linking during single stage

[Timothy Arceri]

On 30/10/18 10:14 pm, Tapani Pälli wrote:
> Hi;
> 
> On 10/30/18 1:28 AM, Timothy Arceri wrote:
>> On 29/10/18 10:05 pm, Vadim Shovkoplias wrote:
>>> Hi Timothy,
>>>
>>> Thanks for the review. Piglit patch is updated with the additional 
>>> out var: https://patchwork.freedesktop.org/patch/258899/
>>> Original reporter confirmed that issue is finally fixed with the 
>>> current patch and closed it.
>>>
>>> Can I ask to push the patch please ?
>>
>> I've pushed both. Thanks for the patches!
> 
> Unfortunately it seems this patch introduced regression in Intel CI, I'm 
> getting following failures after this commit (7d66eddbbde):
> 
> piglit.spec.glsl-1_30.execution.tex-miplevel-selection texturelodoffset 
> 2dshadow.snbm64
> piglit.spec.glsl-1_30.execution.tex-miplevel-selection texturelod 
> 2dshadow.snbm64
> piglit.spec.glsl-1_30.execution.tex-miplevel-selection texturelod 
> cube.snbm64
> generated.gpu-hang-otc-gfxtest-snbgt2-02.snbm64

I don't have a Sandy Bridge to test with but its really strange that 
this would break only one generation of hardware. Are you sure it was 
this patch?

> 
> 
>>>
>>> Regards,
>>> Vadym
>>>
>>> сб, 27 окт. 2018 г. в 1:21, Timothy Arceri <tarceri at itsqueeze.com 
>>> <mailto:tarceri at itsqueeze.com>>:
>>>
>>>     On Wed, Oct 24, 2018, at 3:28 AM, Vadym Shovkoplias wrote:
>>>      > Since out variables are copied from shader objects instruction
>>>      > streams to linked shader instruction steam it should be cloned
>>>      > at first to keep source instruction steam unaltered.
>>>      >
>>>      > Fixes: 966a797e433 glsl/linker: Link all out vars from a shader
>>>      > objects on a single stage
>>>      > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105731
>>>      > Signed-off-by: Vadym Shovkoplias
>>>     <vadym.shovkoplias at globallogic.com
>>>     <mailto:vadym.shovkoplias at globallogic.com>>
>>>
>>>     Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com
>>>     <mailto:tarceri at itsqueeze.com>>
>>>
>>>     Also please either update the existing piglit or add a new one to
>>>     better cover the use of the freed vars. From the bug report it seems
>>>     adding another out var is enough to do it. Thanks.
>>>
>>>      > ---
>>>      >  src/compiler/glsl/linker.cpp | 3 ++-
>>>      >  1 file changed, 2 insertions(+), 1 deletion(-)
>>>      >
>>>      > diff --git a/src/compiler/glsl/linker.cpp
>>>     b/src/compiler/glsl/linker.cpp
>>>      > index 7db34ebf95..8b1b03322a 100644
>>>      > --- a/src/compiler/glsl/linker.cpp
>>>      > +++ b/src/compiler/glsl/linker.cpp
>>>      > @@ -2269,10 +2269,11 @@ link_output_variables(struct
>>>     gl_linked_shader
>>>      > *linked_shader,
>>>      >           if (ir->ir_type != ir_type_variable)
>>>      >              continue;
>>>      >
>>>      > -         ir_variable *const var = (ir_variable *) ir;
>>>      > +         ir_variable *var = (ir_variable *) ir;
>>>      >
>>>      >           if (var->data.mode == ir_var_shader_out &&
>>>      >                 !symbols->get_variable(var->name)) {
>>>      > +            var = var->clone(linked_shader, NULL);
>>>      >              symbols->add_variable(var);
>>>      >              linked_shader->ir->push_head(var);
>>>      >           }
>>>      > --
>>>      > 2.18.0
>>>      >
>>>