[Mesa-dev] [PATCH] glsl: disallow mixed varying types within a location

[Ilia Mirkin]

On Wed, Sep 6, 2017 at 12:13 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
>
> On 06/09/17 11:59, Ilia Mirkin wrote:
>>
>> On Tue, Sep 5, 2017 at 9:54 PM, Timothy Arceri <tarceri at itsqueeze.com>
>> wrote:
>>>
>>>
>>> On 06/09/17 11:23, Ilia Mirkin wrote:
>>>>
>>>>
>>>> The enhanced layouts spec has all kinds of restrictions about what can
>>>> and cannot be mixed in a location. Integer/float(/presumably double)
>>>> can't occupy a single location, interpolation has to be the same, as
>>>> well as auxiliary storage (including patch!).
>>>>
>>>> The implication of this is ... don't specify explicit
>>>> locations/components
>>>> if you want better packing, since the auto-packer doesn't care at all
>>>> about most of these restrictions. Sad.
>>>
>>>
>>>
>>> There are still use cases such as SSO, tessellation shaders and varyings
>>> used by interpolateAt (although we just enable the enhanced layout
>>> packing
>>> rules by default for those anyway) were we cannot use the auto-packer.
>>>
>>> As far as the patch goes this should really be in link_varyings.cpp
>>> rather
>>> than linker.cpp, also there is already related validation code in
>>> cross_validate_outputs_to_inputs() any reason for not just modifying the
>>> code there?
>>
>>
>> This applies to whole shader stages. So e.g. in SSO, you still
>> validate the inputs and the outputs. Similarly, you do this for vertex
>> shader inputs.
>
>
> I'm not following what you are trying to say here.
> cross_validate_outputs_to_inputs() does almost the same thing you are doing
> here, but also validates the outputs from one stage with the input from
> another. You just need to adjust the offsets for patches like you have done
> here and add the missing interpolation checks etc, the base type is already
> validated there.
>
>>
>> Ideally it'd be done earlier on, but we need to wait for the interface
>> types to go away, or else it'd be a disaster.
>>
>> Most of link_varyings is concerned with inter-stage logic. It could be
>> moved there, of course, just didn't really seem to belong.
>
>
> link_varyings should be used for all things varyings unless it really
> doesn't make sense. There is no reason to dump everything in linker.cpp. The
> only reason there are still bits in linker.cpp is because Paul left the
> project in the middle of re-factoring things and beside a single patch from
> me that moved more varying related code here a while ago, there has been
> pretty much zero effort in re-factoring any of the GLSL IR compiler into
> more sensible pieces.
>

OK, I'll redo it. I originally had hopes of doing it much earlier in
the flow, but then realized that I needed the interface lowering.