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

[Tapani Pälli]

On 10/31/18 7:28 AM, Tapani Pälli wrote:
> 
> 
> On 10/31/18 12:57 AM, Timothy Arceri wrote:
>> 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?
> 
> Well .. not 100% sure but it looks like previous commit won't show these 
> fails, it is strange. I'll do some runs today and try to figure it out.

Yeah, it passes now so it seems this was some kind of 'CI noise'.

> 
>>>
>>>
>>>>>
>>>>> 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
>>>>>      >
>>>>>