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.pitoi...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
