Bug 103098

Ilia has a partial fix, but no more time to work on it.  Someone could pick it up and finish it:
https://github.com/imirkin/mesa/commit/4d9d4c44eb38f3f5573e33043e1fed608ce910a9

There's an updated version of the patch, that fixes the full test.


https://github.com/imirkin/mesa/commit/2e47b59970bc05e69973b1429fe6cc0890e607c1

Ilia, any chance to submit this patch for review?

It is fixing the full test in Intel. I know that still fails on Nouveau, and you plan to refactor part of it.

But wondering if it is possible to submit it and continue with the work in a new patch.

Hmmm... well, I had left a TODO of refactoring get_varying_type, which I copied from somewhere.

Also, TBH, I have some recollection of it breaking something else.

Oh right -- so with like a TCS input, "vec4 foo[32]", are we supposed to just have a "foo" resource or a "foo[0]" resource (and/or a "foo[1]" resource? I think that tripped me up last (and some tests were failing as a result).

Did you run that (second) patch through a full test run that includes dEQP?

Either way, feel free to send it out with fixes or whatever. Like Ken mentioned, I'm out of time to work on this.

(In reply to Ilia Mirkin from comment #4)
> 
> Did you run that (second) patch through a full test run that includes dEQP?
> 

No, I didn't run full tests. But I'll do.

I've submitted patch https://patchwork.freedesktop.org/series/32757/ to fix this issue.


It is mostly Ilia patch, without the get_varying_type() function.

I've checked and it doesn't cause any regression in piglit, dEQP or GL-CTS.


Also, this varying_structure_locations test has a bug when checking the varyings. So besides this Mesa patch, I also submitted https://gerrit.khronos.org/#/c/1889/ to fix it.


With both patches this test should pass in Mesa/Intel.

(In reply to Ilia Mirkin from comment #4)
>
> Oh right -- so with like a TCS input, "vec4 foo[32]", are we supposed to
> just have a "foo" resource or a "foo[0]" resource (and/or a "foo[1]"
> resource? I think that tripped me up last (and some tests were failing as a
> result).
> 
>

According to spec:

"For an active variable declared as an array of basic types, a single
entry will be generated, with its name string formed by concatenating
the name of the array and the string "[0]". "


I understand this means we would have "foo[0]" only.

(In reply to Juan A. Suarez from comment #7)
> (In reply to Ilia Mirkin from comment #4)
> >
> > Oh right -- so with like a TCS input, "vec4 foo[32]", are we supposed to
> > just have a "foo" resource or a "foo[0]" resource (and/or a "foo[1]"
> > resource? I think that tripped me up last (and some tests were failing as a
> > result).
> > 
> >
> 
> According to spec:
> 
> "For an active variable declared as an array of basic types, a single
> entry will be generated, with its name string formed by concatenating
> the name of the array and the string "[0]". "
> 
> 
> I understand this means we would have "foo[0]" only.

But is it really considered to be an array, given that TCS & co imply a level of arrayness for everything? Maybe :) I just don't know.

Pushed

commit d5a641106baae2122cc3f09b4a755077d902ee88
Author: Juan A. Suarez Romero <jasuarez@igalia.com>
Date:   Tue Oct 31 17:39:17 2017 +0000

    glsl: add varying resources for arrays of complex types
    
    This patch is mostly a patch done by Ilia Mirkin.
    
    It fixes KHR-GL45.enhanced_layouts.varying_structure_locations.
    
    v2: fix locations for TCS/TES/GS inputs and outputs (Ilia)
    
    CC: Ilia Mirkin <imirkin@alum.mit.edu>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103098
    Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
    Signed-off-by: Juan A. Suarez Romero <jasuarez@igalia.com>

 src/compiler/glsl/linker.cpp | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 4 deletions(-)