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

Wed Apr 19 22:47:52 UTC 2017

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?

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