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

[Marek Olšák]

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.

Marek