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

Wed Apr 19 09:14:08 UTC 2017

On 19.04.2017 09:51, Samuel Pitoiset wrote:
>
>
> On 04/18/2017 11:26 PM, Nicolai Hähnle wrote:
>> 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).
>
> Well, it's not *only* about the explicit conversions,
> ARB_bindless_texture allows more flexibility for sampler/image types.
>
> An oversight? Unfortunately, it looks like to me it's not the only one...

Yes, it does look like several places in the spec could use some 
clarification...

>>>> 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?
>
> Yes, contains_opaque() not is_opaque(). Well, how do you plan to handle
> the fact that bindless sampler types are 64-bit? It seemed quite logical
> to make glsl_type::is_64bit() returns true for them.

So, for the purposes of src/compiler/glsl/, I think it simply shouldn't 
matter whether is_64bit returns true or not. However, I agree that for 
st/glsl_to_tgsi, it'd probably be very helpful to make is_64bit return 
true for them.

> I don't remember all the restrictions to be honest (but they are many
> others) because this solution has been adopted for weeks. But the way
> the compiler is implemented, doing the other solution *really* need a
> ton of changes.
>
>>
>>
>>>> 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?
>
> Yeah, this should compile. f() has to be duplicated, one with bound, the
> other one with bindless, that's it. If we do agree about the function
> explosion as Ilia said, it's all good.
>
>>
>> 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.
>
> How? bindless_sampler/bound_sampler are *not* allowed if
> ARB_bindless_texture is not enabled. This can't happen.

Right, the layout qualifiers are not allowed. Effectively, all uniforms 
are bound_sampler without the extension.

But again, those qualifiers are attributes of *uniforms* only! Even 
unextended GLSL allows sampler-typed function parameters.

>> 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 :-)
>
> Yeah. :)
>
> I tried the alternative(s). They have different pros/cons, no one looks
> "perfect".
>
> By the way, I have asked like 2 months ago about this concern on the
> list, no answer. Thus, I discussed with Dave, Timothy and Ilia over IRC
> and the solution happened.

Yeah, I'm sorry I didn't get involved on this one earlier :-(

Cheers,
Nicolai

>
> https://lists.freedesktop.org/archives/mesa-dev/2017-February/145541.html
>
>>
>>
>>>> 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.