[Mesa-dev] [PATCH V4 2/6] glsl: assign hidden uniforms their slot id earlier

[Timothy Arceri]

On Tue, 2015-10-13 at 12:20 +0200, Marek Olšák wrote:
> On Tue, Oct 13, 2015 at 10:13 AM, Timothy Arceri <
> t_arceri at yahoo.com.au> wrote:
> > On Mon, 2015-10-12 at 01:06 +0200, Marek Olšák wrote:
> > > On Sun, Oct 11, 2015 at 9:20 AM, Timothy Arceri <
> > > t_arceri at yahoo.com.au> wrote:
> > > > On Sat, 2015-10-10 at 18:06 +0200, Marek Olšák wrote:
> > > > > Hi Timothy,
> > > > > 
> > > > > One of these 3 commits breaks compilation for Talos shaders
> > > > > with
> > > > > gallium. My piglit patch "glsl-1.30/sampler-bug: ..."
> > > > > contains a
> > > > > minimal test case. I can't say which commit, because Mesa
> > > > > fails
> > > > > to
> > > > > build between them.
> > > > >  It has something to do with uniforms, structures,
> > > > > and samplers.
> > > > > 
> > > > > commit dcd9cd03837545055ce2a315e7e8840cc3254d1a
> > > > > Author: Timothy Arceri <t_arceri at yahoo.com.au>
> > > > > Date:   Sun Aug 30 12:50:34 2015 +1000
> > > > > 
> > > > >     glsl: store uniform slot id in var location field
> > > > > 
> > > > 
> > > > Hi Marek,
> > > > 
> > > > The piglit test passes on my intel based laptop I can't test on
> > > > anything else until later this week (as I'm traveling) but my
> > > > guess
> > > > is
> > > > its something to do with the above patch.
> > > > 
> > > > The other two patches shouldn't change anything for gallium
> > > > drivers
> > > > "glsl: assign hidden uniforms their slot id earlier" just
> > > > assigns
> > > > hidden uniforms their slot id earlier and there shouldn't be
> > > > any
> > > > difference once the IR gets to glsl_to_tgsi.
> > > > 
> > > > Also "glsl: order indices for samplers inside a struct array"
> > > > shouldn't
> > > > change things either as in your test the sampler are not inside
> > > > the
> > > > struct.
> > > > 
> > > > There is some code in the glsl_to_tgsi pass that looks like the
> > > > location field would have always been -1 for uniforms other the
> > > > UBO
> > > > and
> > > > UBO members maybe this has something to do with the problem now
> > > > that
> > > > all uniforms now get a non -1 value.
> > > > 
> > > >       case ir_var_uniform:
> > > >          entry = new(mem_ctx) variable_storage(var,
> > > > PROGRAM_UNIFORM,
> > > >                                                var
> > > > ->data.location);
> > > >          this->variables.push_tail(entry);
> > > >          break;
> > > > 
> > > > I hope this helps get you started. If you haven't figured it
> > > > out by
> > > > later in the week than I'll take a look on my desktop once I
> > > > get
> > > > home.
> > > 
> > > The problem is that _mesa_get_sampler_uniform_value returns a
> > > sampler
> > > index which is greater than 16. If I allow large sampler indices,
> > > it
> > > starts to fail with sampler.file==TGSI_FILE_NULL in the TGSI
> > > codegen
> > > later. I didn't investigate further.
> > > 
> > > Marek
> > 
> > 
> > So from my bisect run your test was passing until:
> > 
> > a907b5dd162b7911b8c21f6d54837831bc078059 is the first bad commit
> > commit a907b5dd162b7911b8c21f6d54837831bc078059
> > Author: Marek Olšák <marek.olsak at amd.com>
> > Date:   Mon Oct 5 03:26:48 2015 +0200
> > 
> >     st/mesa: translate fragment shaders into TGSI when we get them
> > 
> >     Reviewed-by: Dave Airlie <airlied at redhat.com>
> >     Reviewed-by: Brian Paul <brianp at vmware.com>
> >     Tested-by: Brian Paul <brianp at vmware.com>
> 
> It's a linker test, so you really need to set ST_DEBUG=precompile to
> get that shader translated before this commit. Using --enable-debug
> should make it fail an assertion earlier without ST_DEBUG=precompile.
> 
> You didn't need to bisect. I had bisected the bug already and it's
> one
> of your 3 commits.

I did a bisect because the test passed at my commits but not in master.
It would have helped if you included the information about needing to
use ST_DEBUG=precompile in the original email. 

The test passed even with --enable-debug, anyway I now get the error
using ST_DEBUG=precompile so I'll take a better looking into the
problem tomorrow.

> 
> Marek