[Mesa-dev] [PATCH 11/19] glsl: Track explicitly set location independent from the actual location

[Ian Romanick]

On 04/20/2014 04:53 PM, Eric Anholt wrote:
> Ian Romanick <idr at freedesktop.org> writes:
> 
>> On 04/11/2014 04:35 PM, Eric Anholt wrote:
>>> Ian Romanick <idr at freedesktop.org> writes:
>>>
>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>>
>>>> Almost all of the time the location set by layout(location=...) is the
>>>> location that will be used for the variable.  Vertex shader inputs and
>>>> fragment shader outputs, for example, are visible to the API.  We just
>>>> use those actual settings.
>>>>
>>>> Locations set for varyings, however, are a different story.  In those
>>>> cases, the locations set in the shader are just used to facilitate
>>>> matching outputs to inputs.  We need to track the value so that we can
>>>> ensure that
>>>>
>>>>    layout(location=2) out float foo;
>>>>
>>>> will be assigned the same resource as
>>>>
>>>>    layout(location=2) in float bar;
>>>>
>>>> but we probably don't want to use 2 as the actual location in the set of
>>>> varyings.
>>>
>>> I assumed that layout(location=2) would be using VARYING_SLOT_VARx -- I
>>> don't know of any reason to try and pack these, and it sucks to see
>>> another field in our already-bloated ir_variables.  It looks like other
>>> code in the series would go away if we just mapped things directly to
>>> VARYING_SLOTs.
>>
>> I started off with that approach, and I encountered a couple problems.
>> Since you can have a mix of varyings with and without explicit
>> locations, it made the code for resetting the locations during link more
>> complex.  I recall thinking that the changes should have been simple and
>> obvious, but they weren't.  It was a few months ago at this point, so I
>> don't recall the details.
>>
>> The other problem was that the i965 backend makes assumptions about
>> there not being holes in the set of varyings used.  When shaders ended
>> up using, say, VARYING_SLOT_VAR0 and VARYING_SLOT_VAR1 (but not
>> VARYING_SLOT_VAR2), I started hitting assertions in the backend.  It
>> seemed easier and less obtrusive to have the frontend continue giving
>> the backend shaders that matched its assumptions.
> 
> There shouldn't be anything requiring that varying slots used be
> contiguous.  For example, someone could use gl_TexCoord[0] and
> gl_TexCoord[2] today, and there should be an open slot between the two.

Yeah... my memory was a bit faulty.  The problem wasn't in the
backend.  The problem was in lower_packed_varyings.  I also believe
that lower_packed_varyings would split and repack gl_TexCoord in your
example. :) I removed user_location (just using the explicit_location
flag and location), and it hits:

arb_separate_shader_object-rendezvous_by_location: ../../src/glsl/lower_packed_varyings.cpp:546: ir_dereference* {anonymous}::lower_packed_varyings_visitor::get_packed_varying_deref(unsigned int, ir_variable*, const char*, unsigned int): Assertion `slot < locations_used' failed.

I'm pretty sure this is the same assertion that I was hitting before.
Removing the calls to lower_packed_varyings from the linker avoids the
assertion (duh), and all the SSO rendering tests still pass.

All of the transform feedback code assumes that varyings are packed.
Without lower_packed_varyings, I hit:

ext_transform_feedback-interleaved: gen7_sol_state.c:148: gen7_upload_3dstate_so_decl_list: Assertion `vue_map->varying_to_slot[varying] >= 0' failed.

I haven't verified this yet, but I assume that if we just pack varyings
that don't have an explicit location the backend XFB code will still
assert when a varying with a location is used for XFB.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140426/7f0f1b8b/attachment.sig>