<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, May 8, 2017 at 3:26 AM, Nicolai Hähnle <<a href="mailto:nhaehnle@gmail.com" target="_blank">nhaehnle@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 08.05.2017 12:11, Nicolai Hähnle wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On 06.05.2017 20:09, Jason Ekstrand wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On May 6, 2017 7:56:02 AM Samuel Pitoiset <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> ARB_bindless_texture allows to declare image types inside structures, </blockquote> Of course it does... It's not like having samplers and uniforms in structures is a massive source of pain or anything like that... <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> which means we need to keep track of the format. </blockquote> Right. Does it make more sense to put this on the structure element or on the image type itself? SPIR-V makes the format part of the image type. Doing so would also let us potentially get rid of the field in [n]ir_variable. </blockquote> >From a type-theory point of view, I'd say yes. On the other hand, this leads to a massive explosion in the number of types, which doesn't play well with how GLSL overloads work. I really don't think we should pre-generate all the possible type combinations (probably > 1000 types for every texture target) *and* have duplicate intrinsic signatures for all of them. </blockquote></blockquote><div> </div><div>Yes, intrinsic signatures would be a pain. I think I'm won over by that argument. We could do some sort of "interface type" thing where we have one type that's used for actually decoding the image and another type that's used for function signature matching. However, that seems a bit insane. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Long-term, moving to having the format (and other qualifiers!) part of the type is a good idea. But before doing that, we'd have to seriously restructure how GLSL overloads work. </blockquote> Another "on the other hand" is that it's a good idea for the compiler to mirror the spec, and the spec doesn't think of memory and format qualifiers as part of the type. E.g. you can say </blockquote><div> </div><div>Which spec? :-) The GLSL types system is shared by two different front-ends and SPIR-V definitely makes format part of the type. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> foo = texture(sampler2D(handle), ...); but there isn't even grammar to say something like foo = imageLoad(layout(format=r32f) image2D(handle), ...); so a round-trip via variables is necessary. </blockquote><div> </div><div>Yeah, that's ugly... </div><div>As I said above, I think I agree that this is the best we can do for now. Hopefully, we'll have the opportunity to refactor some things and fix that in the future. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This is arguably a corner where the spec is just horrible, but it is what it is. Cheers, Nicolai<div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> In the meantime, these two patches look reasonable to me: Reviewed-by: Nicolai Hähnle <<a href="mailto:nicolai.haehnle@amd.com" target="_blank">nicolai.haehnle@amd.com</a>> Cheers, Nicolai <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Signed-off-by: Samuel Pitoiset <<a href="mailto:samuel.pitoiset@gmail.com" target="_blank">samuel.pitoiset@gmail.com</a>> --- src/compiler/glsl/builtin_variables.cpp | 1 + src/compiler/glsl_types.cpp | 3 +++ src/compiler/glsl_types.h | 8 +++++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/builtin_variables.cpp b/src/compiler/glsl/builtin_variables.cpp index a45c9d62c7..ce4dd43730 100644 --- a/src/compiler/glsl/builtin_variables.cpp +++ b/src/compiler/glsl/builtin_variables.cpp @@ -341,6 +341,7 @@ per_vertex_accumulator::add_field(int slot, const glsl_type *type, this->fields[this->num_fields].memory_coherent = 0; this->fields[this->num_fields].memory_volatile = 0; this->fields[this->num_fields].memory_restrict = 0; + this->fields[this->num_fields].image_format = 0; this->fields[this->num_fields].explicit_xfb_buffer = 0; this->fields[this->num_fields].xfb_buffer = -1; this->fields[this->num_fields].xfb_stride = -1; diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 00a95d4a8e..a495366919 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -955,6 +955,9 @@ glsl_type::record_compare(const glsl_type *b, bool match_locations) const if (this->fields.structure[i].memory_restrict != b->fields.structure[i].memory_restrict) return false; + if (this->fields.structure[i].image_format + != b->fields.structure[i].image_format) + return false; if (this->fields.structure[i].precision != b->fields.structure[i].precision) return false; diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h index 0449087238..67c152119f 100644 --- a/src/compiler/glsl_types.h +++ b/src/compiler/glsl_types.h @@ -978,6 +978,11 @@ struct glsl_struct_field { unsigned memory_restrict:1; /** + * Layout format, applicable to image variables only. + */ + unsigned image_format:16; + + /** * Any of the xfb_* qualifiers trigger the shader to be in transform * feedback mode so we need to keep track of whether the buffer was * explicitly set or if its just been assigned the default global value. @@ -992,7 +997,8 @@ struct glsl_struct_field { sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0), precision(GLSL_PRECISION_NONE), memory_read_only(0), memory_write_only(0), memory_coherent(0), memory_volatile(0), - memory_restrict(0), explicit_xfb_buffer(0), implicit_sized_array(0) + memory_restrict(0), image_format(0), explicit_xfb_buffer(0), + implicit_sized_array(0) { /* empty */ } -- 2.12.2 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote> _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote> </blockquote> -- Lerne, wie die Welt wirklich ist, Aber vergiss niemals, wie sie sein sollte. </div></div></blockquote></div> </div></div>