[Mesa-dev] [RFC PATCH 03/26] glsl: introduce new base types for bindless samplers/images

[Nicolai Hähnle]

On 18.04.2017 21:49, Dave Airlie wrote:
> On 19 April 2017 at 05:30, Samuel Pitoiset <samuel.pitoiset at gmail.com> wrote:
>>
>>
>> On 04/18/2017 08:14 PM, Nicolai Hähnle wrote:
>>>
>>> On 11.04.2017 18:48, Samuel Pitoiset wrote:
>>>>
>>>> Bindless sampler/image types are really different from the existing
>>>> sampler/image types. They are considered 64-bit unsigned integers,
>>>> they can be declared as temporary, shader inputs/outputs and are
>>>> non-opaque types.
>>>>
>>>> For these reasons, it looks more convenient to introduce new
>>>> internal base types to the GLSL compiler, called
>>>> GLSL_TYPE_BINDLESS_SAMPLER and respectively GLSL_TYPE_BINDLESS_IMAGE.
>>>
>>>
>>> Sorry for taking so long to get to this series, but could you explain the
>>> rationale here a bit more?
>>
>>
>> No worries, all feedbacks are always welcome, even late. :)
>>
>> So, the bindless sampler/image types introduced in ARB_bindless_texture are
>> really different from the "standard" ones.
>>
>> They are considered as 64-bit unsigned integers (not 32-bit) and they are
>> non-opaque types. The latter means they can be declared as temporary
>> variables, as shader inputs/outputs, inside an interface block, etc.

I don't know that that's the best way to think about it at the GLSL 
level. Mostly they're still opaque types, it's just that they can be 
explicitly converted to/from 64-bit ints (or uvec2... not having an 
interaction with GL_ARB_gpu_shader_int64 seems like a spec oversight).


>> That said, the current sampler/image types are opaque (cf,
>> glsl_type::is_opaque()) and it seemed quite impossible to change the
>> glsl_type helpers to fit my needs.

I see no is_opaque, maybe you mean contains_opaque? I agree that it's 
annoying that the restrictions expressed in ast_to_hir.cpp need to be 
modified. What other helpers are an issue?


>> I tried many different solutions before figuring this one which seems better
>> for some reasons:
>>
>> - easy to make bindless sampler/image types 64-bit unsigned int
>> - easy to make bindless sampler/image types non-opaque
>> - should avoid breakage because the base type is different
>> - reduce the amount of changes in most places in the compiler

I still don't see a *positive* justification for having the two 
different type families. Where do you actually need to *distinguish* 
between bindless and bound samplers in the compiler? The spec really 
doesn't make that distinction at all, except in one single place:

    "These modifiers control whether default-block uniforms of
     the corresponding types may have their values set via both
     UniformHandle* and Uniform1i (bindless_sampler and
     bindless_image) or only via Uniform1i (bound_sampler and
     bound_image)."

It seems like the only place to distinguish between the two is actually 
*outside* the compiler, in the uniforms API. And that's not even really 
have about the *type* of the variable; it's about the variable's memory 
layout. So it really behaves more similarly to the layout qualifiers 
(offset, alignment) you have in SSBO and UBO blocks rather than a 
property of a type.

So as I see it, this code is messing with the type system for no good 
reason, and doing so in a fundamentally incoherent way that goes against 
all that is good and sane in compiler design. And I think it shows. 
Consider the following example shader:

    layout (bound_sampler) uniform sampler2D tex_bound;
    layout (bindless_sampler) uniform sampler2D tex_bindless;

    vec4 f(sampler2D s, ...)
    {
       return texture2D(s, ...);
    }

    void main()
    {
       vec4 a = f(tex_bound, ...);
       vec4 b = f(tex_bindless, ...);
    }

What's the type of the parameter `s' of function `f'? Will this compile, 
and if so, how?

I guess the answer is: as is, the type is sampler2DBindless, which makes 
no sense whatsoever if the function f appears in a shader which 
*doesn't* enable ARB_bindless_texture (which is allowed!). But having 
the type of f depend on whether the extension is enabled makes even 
*less* sense.

And does it compile? Well, I don't recall seeing code that would 
automatically do a type conversion -- you probably either end up 
generating an incorrect compiler error, or you end up with IR that 
assigns a value of bindless-type to a variable of non-bindless-type, or 
vice versa.

None of this gives me confidence from a compiler design perspective. 
What's left are some vague implementation concerns that I don't 
understand yet, admittedly in part because I haven't tried to implement 
the alternative :-)


>> At the Gallium level, the changes are really small. Basically, if the type
>> is a bindless sampler, the ir_dereference variable is visited and it can be
>> considered as PROGRAM_UNIFORM or PROGRAM_TEMPORARY (for shader
>> inputs/outputs).
>>
>> Hopefully, you are convinced now. :)
>
> When I did my initial implementation I went with merged types, I don't think
> it was a good idea either. Apart from making old non-bindless samples take up
> twice as much space, there's a lot or corner cases.

Non-bindless samplers taking up twice as much space for the accounting 
of the default uniform block is something the spec explicitly calls out 
as being okay, though.

Even if we decide that it isn't, it's really something that belongs into 
struct gl_uniform_storage during linking, because it's clearly a 
property of... uniform storage layout.


> Unless we hit some problem in the future where they need to be the same
> type I'd say this is preferable.

I hope the code sample above is convincing evidence that using different 
types for the same thing just makes no sense.

Cheers,
Nicolai


>
> Dave.
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.