<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> On 05/10/18 17:44, Jason Ekstrand wrote: <blockquote type="cite" cite="mid:CAOFGe96CDxupW96g+SrAyvdS9cCC+LV9bPDB+kqi6oWR6dP8OA@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=utf-8"> <div dir="ltr"> <div class="gmail_quote"> <div dir="ltr">On Fri, Oct 5, 2018 at 10:34 AM Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com" moz-do-not-send="true">apinheiro@igalia.com</a>> wrote: </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 05/10/18 16:13, Jason Ekstrand wrote: > This is different from the GL_ARB_spirv pass because it generates a much > simpler data structure that isn't tied to OpenGL and mtypes.h. I have just skimmed it (don't have time right now for a full check, will take a deeper look next Monday), but FWIW, the GL_ARB_spirv pass does a initial mtypes-free info gathering (get_active_xfb_varyings) and then it uses that info to fill-up the OpenGL specific mtypes.h. In fact, if we just compare the initial GL_ARB_spirv gathering with this new pass, your pass seems more complete, and with more checks. So I was wondering if it would be possible to remove some code on the GL_ARB_spirv pass and use this new pass instead. Did you check if that would be possible? If not I'm willing to check next week. </blockquote> <div> </div> <div>I didn't really look into that too much. When drafting this one, I did base it somewhat on the GL_ARB_spirv pass so I"m not surprised it's similar. At the time, I was just trying to get something working and wasn't too worried about code duplication. If we can use this pass for GL_ARB_spirv, that'd be fantastic. </div> </div> </div> </blockquote> Ok, then next week I will check for sure if we can do that. <blockquote type="cite" cite="mid:CAOFGe96CDxupW96g+SrAyvdS9cCC+LV9bPDB+kqi6oWR6dP8OA@mail.gmail.com"> <div dir="ltr"> <div class="gmail_quote"> <div>If we do go that route, however, we may want to do something better than assert() for the error handling. </div> </div> </div> </blockquote> Ok, first I will check if this new pass gather the info GL_ARB_spirv pass needs, and see how much the GL_ARB_spirv pass would need to be modified (as the gathering info formatting would change). Then we can talk about what to do with the asserts. <blockquote type="cite" cite="mid:CAOFGe96CDxupW96g+SrAyvdS9cCC+LV9bPDB+kqi6oWR6dP8OA@mail.gmail.com"> <div dir="ltr"> <div class="gmail_quote"> <div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > --- > src/compiler/Makefile.sources | 4 +- > src/compiler/nir/meson.build | 2 + > src/compiler/nir/nir_gather_xfb_info.c | 150 +++++++++++++++++++++++++ > src/compiler/nir/nir_xfb_info.h | 59 ++++++++++ > 4 files changed, 214 insertions(+), 1 deletion(-) > create mode 100644 src/compiler/nir/nir_gather_xfb_info.c > create mode 100644 src/compiler/nir/nir_xfb_info.h > > diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources > index d3b06564832..46ed5e47b46 100644 > --- a/src/compiler/Makefile.sources > +++ b/src/compiler/Makefile.sources > @@ -216,6 +216,7 @@ NIR_FILES = \ > nir/nir_format_convert.h \ > nir/nir_from_ssa.c \ > nir/nir_gather_info.c \ > + nir/nir_gather_xfb_info.c \ > nir/nir_gs_count_vertices.c \ > nir/nir_inline_functions.c \ > nir/nir_instr_set.c \ > @@ -307,7 +308,8 @@ NIR_FILES = \ > nir/nir_validate.c \ > nir/nir_vla.h \ > nir/nir_worklist.c \ > - nir/nir_worklist.h > + nir/nir_worklist.h \ > + nir/nir_xfb_info.h > > SPIRV_GENERATED_FILES = \ > spirv/spirv_info.c \ > diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build > index 090aa7a628f..b416e561eb0 100644 > --- a/src/compiler/nir/meson.build > +++ b/src/compiler/nir/meson.build > @@ -100,6 +100,7 @@ files_libnir = files( > 'nir_format_convert.h', > 'nir_from_ssa.c', > 'nir_gather_info.c', > + 'nir_gather_xfb_info.c', > 'nir_gs_count_vertices.c', > 'nir_inline_functions.c', > 'nir_instr_set.c', > @@ -192,6 +193,7 @@ files_libnir = files( > 'nir_vla.h', > 'nir_worklist.c', > 'nir_worklist.h', > + 'nir_xfb_info.h', > '../spirv/GLSL.ext.AMD.h', > '../spirv/GLSL.std.450.h', > '../spirv/gl_spirv.c', > diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c > new file mode 100644 > index 00000000000..a53703bb9bf > --- /dev/null > +++ b/src/compiler/nir/nir_gather_xfb_info.c > @@ -0,0 +1,150 @@ > +/* > + * Copyright © 2018 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + */ > + > +#include "nir_xfb_info.h" > + > +#include <util/u_math.h> > + > +static void > +add_var_xfb_outputs(nir_xfb_info *xfb, > + nir_variable *var, > + unsigned *location, > + unsigned *offset, > + const struct glsl_type *type) > +{ > + if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) { > + unsigned length = glsl_get_length(type); > + const struct glsl_type *child_type = glsl_get_array_element(type); > + for (unsigned i = 0; i < length; i++) > + add_var_xfb_outputs(xfb, var, location, offset, child_type); > + } else if (glsl_type_is_struct(type)) { > + unsigned length = glsl_get_length(type); > + for (unsigned i = 0; i < length; i++) { > + const struct glsl_type *child_type = glsl_get_struct_field(type, i); > + add_var_xfb_outputs(xfb, var, location, offset, child_type); > + } > + } else { > + assert(var->data.xfb_buffer < NIR_MAX_XFB_BUFFERS); > + if (xfb->buffers_written & (1 << var->data.xfb_buffer)) { > + assert(xfb->strides[var->data.xfb_buffer] == var->data.xfb_stride); > + assert(xfb->buffer_to_stream[var->data.xfb_buffer] == var->data.stream); > + } else { > + xfb->buffers_written |= (1 << var->data.xfb_buffer); > + xfb->strides[var->data.xfb_buffer] = var->data.xfb_stride; > + xfb->buffer_to_stream[var->data.xfb_buffer] = var->data.stream; > + } > + > + assert(var->data.stream < NIR_MAX_XFB_STREAMS); > + xfb->streams_written |= (1 << var->data.stream); > + > + unsigned comp_slots = glsl_get_component_slots(type); > + unsigned attrib_slots = DIV_ROUND_UP(comp_slots, 4); > + assert(attrib_slots == glsl_count_attribute_slots(type, false)); > + > + /* Ensure that we don't have, for instance, a dvec2 with a location_frac > + * of 2 which would make it crass a location boundary even though it > + * fits in a single slot. However, you can have a dvec3 which crosses > + * the slot boundary with a location_frac of 2. > + */ > + assert(DIV_ROUND_UP(var->data.location_frac + comp_slots, 4) == attrib_slots); > + > + assert(var->data.location_frac + comp_slots <= 8); > + uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; > + > + assert(attrib_slots <= 2); > + for (unsigned s = 0; s < attrib_slots; s++) { > + nir_xfb_output_info *output = &xfb->outputs[xfb->output_count++]; > + > + output->buffer = var->data.xfb_buffer; > + output->offset = *offset; > + output->location = *location; > + output->component_mask = (comp_mask >> (s * 4)) & 0xf; > + > + (*location)++; > + *offset += comp_slots * 4; > + } > + } > +} > + > +static int > +compare_xfb_output_offsets(const void *_a, const void *_b) > +{ > + const nir_xfb_output_info *a = _a, *b = _b; > + return a->offset - b->offset; > +} We have this same comparison on link_varyings.c and gl_nir_link_xfb. Not sure how I feel about having the same function three times. I would really hope that my previous comment to reuse code to be true. </blockquote> <div> </div> <div>Yeah, if we can re-use some stuff, that'd be great. On the other hand, it's six lines of code which ammount to one interesting line so I'm not too worried about the duplication. :-)</div> <div> </div> <div>--Jason </div> <div> </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > +nir_xfb_info * > +nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) > +{ > + assert(shader->info.stage == MESA_SHADER_VERTEX || > + shader->info.stage == MESA_SHADER_TESS_EVAL || > + shader->info.stage == MESA_SHADER_GEOMETRY); > + > + /* Compute the number of outputs we have. This is simply the number of > + * cumulative locations consumed by all the variables. If a location is > + * represented by multiple variables, then they each count separately in > + * number of outputs. > + */ > + unsigned num_outputs = 0; > + nir_foreach_variable(var, &shader->outputs) { > + if (var->data.explicit_xfb_buffer || > + var->data.explicit_xfb_stride) { > + assert(var->data.explicit_xfb_buffer && > + var->data.explicit_xfb_stride && > + var->data.explicit_offset); > + num_outputs += glsl_count_attribute_slots(var->type, false); > + } > + } > + if (num_outputs == 0) > + return NULL; > + > + nir_xfb_info *xfb = rzalloc_size(mem_ctx, nir_xfb_info_size(num_outputs)); > + > + /* Walk the list of outputs and add them to the array */ > + nir_foreach_variable(var, &shader->outputs) { > + if (var->data.explicit_xfb_buffer || > + var->data.explicit_xfb_stride) { > + unsigned logcation = var->data.location; > + unsigned offset = var->data.offset; > + add_var_xfb_outputs(xfb, var, &location, &offset, var->type); > + } > + } > + assert(xfb->output_count == num_outputs); > + > + /* Everything is easier in the state setup code if the list is sorted in > + * order of output offset. > + */ > + qsort(xfb->outputs, xfb->output_count, sizeof(xfb->outputs[0]), > + compare_xfb_output_offsets); > + > + /* Finally, do a sanity check */ > + unsigned max_offset[NIR_MAX_XFB_BUFFERS] = { }; > + for (unsigned i = 0; i < xfb->output_count; i++) { > + assert(xfb->outputs[i].offset >= max_offset[xfb->outputs[i].buffer]); > + assert(xfb->outputs[i].component_mask != 0); > + unsigned slots = util_bitcount(xfb->outputs[i].component_mask); > + max_offset[xfb->outputs[i].buffer] = xfb->outputs[i].offset + slots * 4; > + } > + > + return xfb; > +} > diff --git a/src/compiler/nir/nir_xfb_info.h b/src/compiler/nir/nir_xfb_info.h > new file mode 100644 > index 00000000000..9b543df5f47 > --- /dev/null > +++ b/src/compiler/nir/nir_xfb_info.h > @@ -0,0 +1,59 @@ > +/* > + * Copyright © 2018 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > + * IN THE SOFTWARE. > + */ > + > +#ifndef NIR_XFB_INFO_H > +#define NIR_XFB_INFO_H > + > +#include "nir.h" > + > +#define NIR_MAX_XFB_BUFFERS 4 > +#define NIR_MAX_XFB_STREAMS 4 > + > +typedef struct { > + uint8_t buffer; > + uint16_t offset; > + uint8_t location; > + uint8_t component_mask; > +} nir_xfb_output_info; > + > +typedef struct { > + uint8_t buffers_written; > + uint8_t streams_written; > + > + uint16_t strides[NIR_MAX_XFB_BUFFERS]; > + uint8_t buffer_to_stream[NIR_MAX_XFB_STREAMS]; > + > + uint16_t output_count; > + nir_xfb_output_info outputs[0]; > +} nir_xfb_info; > + > +static inline size_t > +nir_xfb_info_size(uint16_t output_count) > +{ > + return sizeof(nir_xfb_info) + sizeof(nir_xfb_output_info) * output_count; > +} > + > +nir_xfb_info * > +nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx); > + > +#endif /* NIR_XFB_INFO_H */ </blockquote> </div> </div> </blockquote> </body> </html>