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

[Tapani Pälli]

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.


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