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

Thu Apr 20 14:54:11 UTC 2017

On 04/20/2017 12:47 AM, Samuel Pitoiset wrote:
> 
> 
> On 04/19/2017 11:14 AM, Nicolai Hähnle wrote:
>> 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.
> 
> One new restriction is glsl_type::component_slots(). With the new base 
> types it's easy to return something similar to uint64_t (ie. 2 
> components if it's BINDLESS_SAMPLER). However, without a separate type 
> it returns 0 for samplers and 1 for images.
> 
> glsl_type is a quite good interface for base types, but for bindless 
> sampler we just can't store the information there. Currently, the only 
> way to know if a sampler is bindless is to have access to a ir_variable 
> object.
> 
> I don't see how to solve this properly without adding hacks. The way 
> glsl_type is designed is one of the argument for the current solution.
> 
> Any thoughts?

It's allowed by the spec to count samplers/images as two components. So 
component_slots() can return N*2 like uint64_t. However, 
glsl_type::components() currently returns 0 for sampler types because 
matrix_columns and vector_elements are 0 (while 1 for image types).

I think this needs to be clarified and components() should return 1 for 
sampler and image types.

> 
>>
>>
>>> 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.
>>>>>
>>>>
>>>>
>>
>>