Re: V3D UIF texture format conversion with TFU
Hi, sorry, I realized that you sent first this email to me privately, but didn't have time to reply. On 1/3/24 14:33, Macoy Madson wrote: Hello, I have been looking at the code in Gallium/drivers/v3d related to texture conversion, specifically v3dx_tfu.c. I have been trying to find how to convert a texture from raster format RGBA8 to UIF (either XOR or NO_XOR). I tried break-pointing the v3dX(tfu) function, but it doesn't appear to be called in my simple textured triangle OpenGL program on a Raspberry Pi 4 (VideoCore VI v. 42). I am looking for where this conversion takes place, as it appears the VideoCore expects textures to be in another format before they can be sampled properly. Is there anyone here who can point me to any one of the following: - How v3dX(tfu) ends up getting used (e.g., a user-space program I can run to blit/convert a raster texture into a UIF texture) - Any notes on the TFU interface itself (what the registers expect, beyond what can clearly infer from v3dX(tfu)) - If the TFU isn't used to convert to UIF, where the conversion takes place I may have misunderstood the texture sampler. I am sampling a texture successfully (pixels are the color I expect) but the pixels are completely "out of order", which is what hints me that they need to be in UIF instead of raster format. I would greatly appreciate someone to bounce a few V3D questions off of. Thanks, Macoy Madson (Apologies if this was received before; I checked the archive and didn't see it, so I subscribed to the list and re-sent in case I need to be a subscriber to send. I sent this to mesa-dev since I am looking for low level details, so if this should go to the mesa list instead please let me know.) For this kind of things it is usually done through gitlab issues. Would it be possible if you create an issue with that, and additionally adding the simple texture triangle program that you mentioned before? Thanks
Re: Re-use of Intel driver genxml files in other project
Hi, On 24/10/23 0:44, Thomas Erbesdobler wrote: Hi everyone, I'm currently working on a very simple OpenCL runtime for Intel GPUs, which I would like to make available as open source project (and maybe also distribute it). To generate the ring-commands (the command stream which controls the GPU, i.e. loads kernels and data, spawns worker threads, etc.) I used the genX.xml-files of the MESA project. They are located at src/intel/genxml in the source tree (i.e. https://gitlab.freedesktop.org/mesa/mesa/-/tree/main/src/intel/genxml). FWIW, some those genxml files are already being used by other drivers. Here a example, with license, on the broadcom case: https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/broadcom/cle/gen_pack_header.py?ref_type=heads#L3 Like MESA itself I am using these genX.xml-files to generate C(++) header files. Most code in that part of MESA's sources is covered by a MIT license, but I did not find a particular license for the genX.xml-files (however the generated headers are MIT-licensed, too). Am I allowed to distribute these genX.xml-files in my project, and if yes, under which terms? I would probably license my OpenCL runtime under the MIT license (SPDX-version), too. Regards, Thomas
Re: VkRunner ported to Rust
On 20/2/23 16:53, Neil Roberts wrote: Hi folks, Hi Neil! Does anybody remember VkRunner? It’s a little tool to help write shader-based tests for Vulkan. It’s the same concept as Piglit’s shader_runner but for Vulkan instead of OpenGL. There are a couple of tests using it in Piglit but apart from that it never really got off the ground. Anyway, I’ve been trying to learn some Rust lately and in order to get some experience working with a non-trivial project I decided to port VkRunner to Rust. I have been trying to find a time slot to learn Rust too. If you have issues around with pending features perhaps one of these days I join your initiative. The port is now complete and available here: https://github.com/bpeel/vkrunner/ It’s a drop-in replacement for the original VkRunner so it should be possible to start using it in a CI system by just changing the git repo, assuming the rust compiler is installed. I was thinking that now that Mesa has some Rust code in it anyway it might not be too unreasonable to expect CI systems to have the Rust compiler available. Other than that there’s not much advantage to using one or the other except for the warm fuzzy feeling knowing that you’re using a project written in a memory-safe language. If the rust-based vkrunner has the same features and can run the same kind of tests that the original c-based vkrunner, I think that using a memory-safe language is an advantage that goes beyond just a warm fuzzy feeling. If that is the case, I think that it is a good idea to replace one with the other. I also took the opportunity to add a whole bunch of unit tests so in theory the Rust port might be more robust. It’s currently using Meson as the build system. In the beginning this was necessary because I did the port gradually and it’s probably the best build system if you have a mix of C and Rust code. Now that the port is complete it’d probably be trivial to start using Cargo instead. I’m not sure which would be better. It’s not using any external crates so it doesn’t really need Cargo for now. Now that the port is complete it might be nice to start adding more features. If anyone else hasn’t taken the plunge to start using Rust yet this might be a nice project to get involved in if you fancy helping. Kind regards, – Neil
[Mesa-dev] [PATCH] docs: advice to resolve discussion on gitlab MR doc
For newcomers to gitlab, it is not evident that it is better to press the "Resolve Discussion" button when you update your branch handling feedback. --- As the commit message says, it is not always evident. I was pointed to do that when I started to use gitlab, and just today I mentioned it to two different people that didn't know about that. Having said so, I feel that the specific text needs some poulishing first, so any suggestion is welcome. docs/submittingpatches.html | 4 1 file changed, 4 insertions(+) diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html index 020e73d09ec..147b97d76e1 100644 --- a/docs/submittingpatches.html +++ b/docs/submittingpatches.html @@ -258,6 +258,10 @@ your email administrator for this.) Make changes and update your branch based on feedback + After an update, for the feedback you handled, close the + feedback discussion with the button "Resolve Discussion". In this + way the reviewer would know which feedback got handled and which + not. Old, stale MR may be closed, but you can reopen it if you still want to pursue the changes You should periodically check to see if your MR needs to be -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] docs: document MESA_GLSL=errors keyword
Added with commit 0161691f3518, still checked on shaderapi.c _mesa_get_shader_flag method. --- docs/shading.html | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/shading.html b/docs/shading.html index 9e3c7218e31..76f25316f86 100644 --- a/docs/shading.html +++ b/docs/shading.html @@ -59,6 +59,7 @@ execution. These are generally used for debugging. nopfrag - force fragment shader to be a simple shader that passes through the color attribute. useprog - log glUseProgram calls to stderr +errors - GLSL compilation and link errors will be reported to stderr. Example: export MESA_GLSL=dump,nopt -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] spirv: handle FragCoord and SamplePosition builtins
Those builtins need to fill origin_upper_left and pixel_center_integer on the nir variable. Those depends on the execution mode, that moved recently to be handled after creating the variables. This commit adds a pass over the fragment shader inputs to set the proper value once we have all the execution mode values. Fixes: e68871f6a ("spirv: Handle constants and types before execution modes") v2: remove superfluous setting of origin_upper_left --- src/compiler/spirv/spirv_to_nir.c | 13 + src/compiler/spirv/vtn_variables.c | 8 ++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 1cbc926c818..6825d23a238 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -4463,6 +4463,19 @@ spirv_to_nir(const uint32_t *words, size_t word_count, vtn_foreach_execution_mode(b, b->entry_point, vtn_handle_execution_mode, NULL); + /* Update variables data that depends on the execution modes */ + if (b->shader->info.stage == MESA_SHADER_FRAGMENT) { + nir_foreach_variable(var, >shader->inputs) { + switch (var->data.location) { + case VARYING_SLOT_POS: /* FragCoord */ +var->data.pixel_center_integer = b->pixel_center_integer; +/* fallthrough */ + case SYSTEM_VALUE_SAMPLE_POS: +var->data.origin_upper_left = b->origin_upper_left; + } + } + } + if (b->workgroup_size_builtin) { vtn_assert(b->workgroup_size_builtin->type->type == glsl_vector_type(GLSL_TYPE_UINT, 3)); diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index f6b458b7e78..d4b64fd962d 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1448,12 +1448,8 @@ apply_var_decoration(struct vtn_builder *b, case SpvBuiltInCullDistance: var_data->compact = true; break; - case SpvBuiltInFragCoord: - var_data->pixel_center_integer = b->pixel_center_integer; - /* fallthrough */ - case SpvBuiltInSamplePosition: - var_data->origin_upper_left = b->origin_upper_left; - break; + /* FragCoord and SamplePosition depends on having the correct value for + * the execution mode. Will be handled later */ default: break; } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/2] spirv/nir: fix pixel_center_integer/origin_upper_left (two different solutions)
Note that the two patches are independent. Are two possible solutions for the same problem. Details below. As mentioned on the following MR: https://gitlab.freedesktop.org/mesa/mesa/merge_requests/144 changing the order of how ExecutionModes are handled affected handling OriginUpperLeft and PixelCenterInteger. On such MR, I proposed a straighforward walking through the inputs after the execution modes were handled. Jason said that solution was ok, but also mentioned that an alternative would be move that info from nir_variable_data to shader_info.fs So this series has those two solutions implemented, so we can choose what we prefer. The first one is a smaller change, that only affects spirv_to_nir. The second is bigger, but at the same time is somewhat a cleaning, because pixel_center_integer was already on shader_info.fs, so right now we have duplicated data. Although I'm waiting for Intel CI to confirm that there are no regressions, both patches fixes the tests that highlighted the problem. -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] nir: move pixel_center_integer/origin_upper_left to shader_info.fs
Although on GLSL those are set using a layout qualifier to gl_FragCoord builtin, they are basically a global mode. In fact, on SPIR-V they are set as an global ExecutionMode, not as a decoration for the builtin. With this change, we are just mapping them more similar to SPIR-V, instead of more similar to GLSL. FWIW, shader_info.fs already had pixel_center_integer, so this change also removes some redundancy. This change was needed because recently spirv_to_nir changed the order in which execution modes and variables are handled, so the variables didn't get the correct values. Now the info is set on the shader itself. Fixes: e68871f6a ("spirv: Handle constants and types before execution modes") --- src/compiler/glsl/glsl_to_nir.cpp | 9 +++-- src/compiler/nir/nir.h | 8 src/compiler/nir/nir_lower_system_values.c | 6 -- src/compiler/nir/nir_lower_wpos_ytransform.c | 4 ++-- src/compiler/shader_info.h | 6 ++ src/compiler/spirv/spirv_to_nir.c | 4 ++-- src/compiler/spirv/vtn_private.h | 2 -- src/compiler/spirv/vtn_variables.c | 6 -- src/intel/blorp/blorp_blit.c | 2 +- src/intel/blorp/blorp_clear.c | 3 ++- src/intel/blorp/blorp_nir_builder.h| 1 - src/intel/vulkan/anv_nir_lower_input_attachments.c | 2 +- src/mesa/program/prog_to_nir.c | 8 13 files changed, 25 insertions(+), 36 deletions(-) diff --git a/src/compiler/glsl/glsl_to_nir.cpp b/src/compiler/glsl/glsl_to_nir.cpp index 09599e4cee7..6ff20e8a692 100644 --- a/src/compiler/glsl/glsl_to_nir.cpp +++ b/src/compiler/glsl/glsl_to_nir.cpp @@ -397,8 +397,13 @@ nir_visitor::visit(ir_variable *ir) } var->data.interpolation = ir->data.interpolation; - var->data.origin_upper_left = ir->data.origin_upper_left; - var->data.pixel_center_integer = ir->data.pixel_center_integer; + /* We only set the values of origin_upper_left and pixel_center_integer if +* they are set, to avoid following variables ovewritting them +*/ + if (ir->data.origin_upper_left) + shader->info.fs.origin_upper_left = ir->data.origin_upper_left; + if (ir->data.pixel_center_integer) + shader->info.fs.pixel_center_integer = ir->data.pixel_center_integer; var->data.location_frac = ir->data.location_frac; switch (ir->data.depth_layout) { diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index ff2c41faf27..bb2d3884acb 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -237,14 +237,6 @@ typedef struct nir_variable { */ unsigned interpolation:2; - /** - * \name ARB_fragment_coord_conventions - * @{ - */ - unsigned origin_upper_left:1; - unsigned pixel_center_integer:1; - /*@}*/ - /** * If non-zero, then this variable may be packed along with other variables * into a single varying slot, so this offset should be applied when diff --git a/src/compiler/nir/nir_lower_system_values.c b/src/compiler/nir/nir_lower_system_values.c index 7c1aa5fa801..68b0ea89c8d 100644 --- a/src/compiler/nir/nir_lower_system_values.c +++ b/src/compiler/nir/nir_lower_system_values.c @@ -254,12 +254,6 @@ convert_block(nir_block *block, nir_builder *b) break; } - case SYSTEM_VALUE_FRAG_COORD: - assert(b->shader->info.stage == MESA_SHADER_FRAGMENT); - b->shader->info.fs.pixel_center_integer = -var->data.pixel_center_integer; - break; - default: break; } diff --git a/src/compiler/nir/nir_lower_wpos_ytransform.c b/src/compiler/nir/nir_lower_wpos_ytransform.c index 444e211b680..34a4801d66b 100644 --- a/src/compiler/nir/nir_lower_wpos_ytransform.c +++ b/src/compiler/nir/nir_lower_wpos_ytransform.c @@ -181,7 +181,7 @@ lower_fragcoord(lower_wpos_ytransform_state *state, * u,h -> l,i: (99.5 + 0.5) * -1 + 100 = 0 */ - if (fragcoord->data.origin_upper_left) { + if (state->shader->info.fs.origin_upper_left) { /* Fragment shader wants origin in upper-left */ if (options->fs_coord_origin_upper_left) { /* the driver supports upper-left origin */ @@ -203,7 +203,7 @@ lower_fragcoord(lower_wpos_ytransform_state *state, } } - if (fragcoord->data.pixel_center_integer) { + if (state->shader->info.fs.pixel_center_integer) { /* Fragment shader wants pixel center integer */ if (options->fs_coord_pixel_center_integer) { /* the driver supports pixel center integer */ diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h index 3d871938751..12f869ebb52 100644 --- a/src/compiler/shader_info.h +++ b/src/compiler/shader_info.h @@ -192,7 +192,13 @@ typedef struct shader_info { bool post_depth_coverage; + /** + * \name
[Mesa-dev] [PATCH 1/1] spirv: handle FragCoord and SamplePosition builtins
Those builtins need to fill origin_upper_left and pixel_center_integer on the nir variable. Those depends on the execution mode, that moved recently to be handled after creating the variables. This commit adds a pass over the fragment shader inputs to set the proper value once we have all the execution mode values. Fixes: e68871f6a ("spirv: Handle constants and types before execution modes") --- src/compiler/spirv/spirv_to_nir.c | 14 ++ src/compiler/spirv/vtn_variables.c | 8 ++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 1cbc926c818..e15ff0ff806 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -4463,6 +4463,20 @@ spirv_to_nir(const uint32_t *words, size_t word_count, vtn_foreach_execution_mode(b, b->entry_point, vtn_handle_execution_mode, NULL); + /* Update variables data that depends on the execution modes */ + if (b->shader->info.stage == MESA_SHADER_FRAGMENT) { + nir_foreach_variable(var, >shader->inputs) { + switch (var->data.location) { + case VARYING_SLOT_POS: /* FragCoord */ +var->data.pixel_center_integer = b->pixel_center_integer; +/* fallthrough */ +var->data.origin_upper_left = b->origin_upper_left; + case SYSTEM_VALUE_SAMPLE_POS: +var->data.origin_upper_left = b->origin_upper_left; + } + } + } + if (b->workgroup_size_builtin) { vtn_assert(b->workgroup_size_builtin->type->type == glsl_vector_type(GLSL_TYPE_UINT, 3)); diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index f6b458b7e78..d4b64fd962d 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1448,12 +1448,8 @@ apply_var_decoration(struct vtn_builder *b, case SpvBuiltInCullDistance: var_data->compact = true; break; - case SpvBuiltInFragCoord: - var_data->pixel_center_integer = b->pixel_center_integer; - /* fallthrough */ - case SpvBuiltInSamplePosition: - var_data->origin_upper_left = b->origin_upper_left; - break; + /* FragCoord and SamplePosition depends on having the correct value for + * the execution mode. Will be handled later */ default: break; } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] nir: remove unused variable
To avoid the following warning: ./src/compiler/nir/nir_loop_analyze.c:807:16: warning: unused variable ‘ns’ [-Wunused-variable] nir_shader *ns = impl->function->shader; --- Perhaps this is solved on any of the loop analysis patches pending to be reviewed, but just in case, sending it. src/compiler/nir/nir_loop_analyze.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/compiler/nir/nir_loop_analyze.c b/src/compiler/nir/nir_loop_analyze.c index 3de45401975..259f02a854e 100644 --- a/src/compiler/nir/nir_loop_analyze.c +++ b/src/compiler/nir/nir_loop_analyze.c @@ -803,7 +803,6 @@ get_loop_info(loop_info_state *state, nir_function_impl *impl) /* Run through each of the terminators and try to compute a trip-count */ find_trip_count(state); - nir_shader *ns = impl->function->shader; nir_foreach_block_in_cf_node(block, >loop->cf_node) { if (force_unroll_heuristics(state, block)) { state->loop->info->force_unroll = true; -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] spirv/nir: adjust location assignment for the case of arrays of blocks
This is needed due how the types get rearranged after the struct splitting. So for example, this array of blocks: layout(location = 0) out block { vec4 v; vec3 v2; } x[2]; Would be splitted on two nir variables with the following types: * vec4 v[2] * vec3 v2[2] So we need to take into account the length of the array to avoid locations overlaps one with the other. --- Hi Jason, again, sending in advance patches, just in case you are working on the same. I was able to fix the location overlapping without all those crazy ideas about lowering array of blocks into individual blocks, by just adjusting the locations as this patch shows. FWIW, the resulting locations are equivalent to those that we get with GLSL IR, that results on a similar splitting. With this change I got the following working: * SPIR-V simple arrays of blocks input/outputs * The arrays of blocks inputs/outputs + interpolator qualifiers test I mentioned to you last week [1] when run its SPIR-V equivalent. * SPIR-V xfb tests using arrays of blocks, where the xfb offset are assigned to all block members. * SPIR-V xfb tests using arrays of blocks, where the xfb offset is assigned to just one member, so just that member is captured, although as many times as the array length (yes! afaiu by spec that needs to work) So now, the only pending thing is a cleanup and send the series to review. Specifically, I think that this series can be put on top of current master instead of the arb_gl_spirv. Will try that and send a final series this week or early next week. BR [1] https://github.com/Igalia/piglit/blob/master/tests/spec/glsl-1.50/execution/interface-block-interpolation-array.shader_test src/compiler/spirv/vtn_variables.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index a8f2fdfa534..87386cee42f 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1672,6 +1672,14 @@ add_missing_member_locations(struct vtn_variable *var, glsl_get_length(glsl_without_array(var->type->type)); int location = var->base_location; + /* To know if it is a interface block we can't ask directly for +* var->type->block because on the case of arrays of blocks, block is set +* on the array_element. +*/ + bool is_array_block = var->var->interface_type != NULL && + glsl_type_is_array(var->type->type); + int adjustment = is_array_block ? glsl_get_length(var->type->type) : 1; + for (unsigned i = 0; i < length; i++) { /* From the Vulkan spec: * @@ -1702,8 +1710,12 @@ add_missing_member_locations(struct vtn_variable *var, const struct glsl_type *member_type = glsl_get_struct_field(glsl_without_array(var->type->type), i); + /* For arrays of interface blocks we can't just add the attribute slots + * of a member type due how the splitting would rearrange the types, so + * we need to adjust for the array length in that case. + */ location += - glsl_count_attribute_slots(member_type, is_vertex_input); + glsl_count_attribute_slots(member_type, is_vertex_input) * adjustment; } } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 13/13] nir/linker: use nir_gather_xfb_info
Instead of a custom ARB_gl_spirv xfb gather info pass. In fact, this is not only about reusing code, but the current custom code was not handling properly how many varyings are enumerated from some complex types. So this change is also about fixing some corner cases. --- src/compiler/glsl/gl_nir_link_xfb.c | 252 1 file changed, 72 insertions(+), 180 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_xfb.c b/src/compiler/glsl/gl_nir_link_xfb.c index bcef1e1863d..b294a4885b0 100644 --- a/src/compiler/glsl/gl_nir_link_xfb.c +++ b/src/compiler/glsl/gl_nir_link_xfb.c @@ -22,8 +22,8 @@ */ #include "nir.h" +#include "nir_xfb_info.h" #include "gl_nir_linker.h" -#include "ir_uniform.h" /* for gl_uniform_storage */ #include "linker_util.h" #include "main/context.h" @@ -34,158 +34,13 @@ * particularities. */ -struct active_xfb_buffer { - GLuint stride; - GLuint num_varyings; -}; - -struct active_xfb_varyings { - unsigned num_varyings; - unsigned num_outputs; - unsigned buffer_size; - struct nir_variable **varyings; - struct active_xfb_buffer buffers[MAX_FEEDBACK_BUFFERS]; -}; - -static unsigned -get_num_outputs(nir_variable *var) -{ - return glsl_count_attribute_slots(var->type, - false /* is_vertex_input */); -} - -static void -add_xfb_varying(struct active_xfb_varyings *active_varyings, -nir_variable *var) -{ - if (active_varyings->num_varyings >= active_varyings->buffer_size) { - if (active_varyings->buffer_size == 0) - active_varyings->buffer_size = 1; - else - active_varyings->buffer_size *= 2; - - active_varyings->varyings = realloc(active_varyings->varyings, - sizeof(nir_variable*) * - active_varyings->buffer_size); - } - - active_varyings->varyings[active_varyings->num_varyings++] = var; - - active_varyings->num_outputs += get_num_outputs(var); -} - static int -cmp_xfb_offset(const void *x_generic, const void *y_generic) -{ - const nir_variable *const *x = x_generic; - const nir_variable *const *y = y_generic; - - if ((*x)->data.xfb_buffer != (*y)->data.xfb_buffer) - return (*x)->data.xfb_buffer - (*y)->data.xfb_buffer; - return (*x)->data.offset - (*y)->data.offset; -} - -static void -get_active_xfb_varyings(struct gl_shader_program *prog, -struct active_xfb_varyings *active_varyings) -{ - for (unsigned i = 0; i < MESA_SHADER_STAGES; ++i) { - struct gl_linked_shader *sh = prog->_LinkedShaders[i]; - if (sh == NULL) - continue; - - nir_shader *nir = sh->Program->nir; - - nir_foreach_variable(var, >outputs) { - if (var->data.explicit_xfb_buffer && - var->data.explicit_xfb_stride) { -assert(var->data.xfb_buffer < MAX_FEEDBACK_BUFFERS); -active_varyings->buffers[var->data.xfb_buffer].stride = - var->data.xfb_stride; - } - - if (!var->data.explicit_xfb_buffer || - !var->data.explicit_offset) -continue; - - active_varyings->buffers[var->data.xfb_buffer].num_varyings++; - - add_xfb_varying(active_varyings, var); - } - } - - /* The xfb_offset qualifier does not have to be used in increasing order -* however some drivers expect to receive the list of transform feedback -* declarations in order so sort it now for convenience. -*/ - qsort(active_varyings->varyings, - active_varyings->num_varyings, - sizeof(*active_varyings->varyings), - cmp_xfb_offset); -} - -static unsigned -add_varying_outputs(nir_variable *var, -const struct glsl_type *type, -unsigned location_offset, -unsigned dest_offset, -struct gl_transform_feedback_output *output) +count_bits(uint8_t mask) { - unsigned num_outputs = 0; - - 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); - unsigned component_slots = glsl_get_component_slots(child_type); - - for (unsigned i = 0; i < length; i++) { - unsigned child_outputs = add_varying_outputs(var, - child_type, - location_offset, - dest_offset, - output + num_outputs); - num_outputs += child_outputs; - location_offset += child_outputs; - dest_offset += component_slots; - } - } 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 =
[Mesa-dev] [RFC PATCH 12/13] nir/xfb_info: handle arrays and AoA of basic types
On OpenGL, a array of a simple type adds just one varying. So gl_transform_feedback_varying_info struct defined at mtypes.h includes the parameters Type (base_type) and Size (number of elements). This commit checks this when the recursive add_var_xfb_outputs call handles arrays, to ensure that just one is addded. We also need to take into account AoA here. v2: take into account basic aoa too --- src/compiler/nir/nir_gather_xfb_info.c | 52 +- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index e3c3376fb34..e19d4908715 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -36,25 +36,61 @@ nir_gather_xfb_info_create(void *mem_ctx, uint16_t output_count, uint16_t varyin return xfb; } +static bool +glsl_type_is_leaf(const struct glsl_type *type) +{ + if (glsl_type_is_struct(type) || + (glsl_type_is_array(type) && +(glsl_type_is_array(glsl_get_array_element(type)) || + glsl_type_is_struct(glsl_get_array_element(type) { + return false; + } else { + return true; + } +} + +static void +add_var_xfb_varying(nir_xfb_info *xfb, +nir_variable *var, +unsigned offset, +const struct glsl_type *type) +{ + nir_xfb_varying_info *varying = >varyings[xfb->varying_count++]; + + varying->type = type; + varying->buffer = var->data.xfb_buffer; + varying->offset = offset; + xfb->buffers[var->data.xfb_buffer].varying_count++; +} + static void add_var_xfb_outputs(nir_xfb_info *xfb, nir_variable *var, unsigned *location, unsigned *offset, unsigned buffer, -const struct glsl_type *type) +const struct glsl_type *type, +bool varying_added) { if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) { unsigned length = glsl_get_length(type); + bool local_varying_added = varying_added; + const struct glsl_type *child_type = glsl_get_array_element(type); + if (!glsl_type_is_array_of_arrays(type) && + glsl_type_is_leaf(child_type)) { + + add_var_xfb_varying(xfb, var, *offset, type); + local_varying_added = true; + } for (unsigned i = 0; i < length; i++) - add_var_xfb_outputs(xfb, var, location, offset, buffer, child_type); + add_var_xfb_outputs(xfb, var, location, offset, buffer, child_type, local_varying_added); } 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, buffer, child_type); + add_var_xfb_outputs(xfb, var, location, offset, buffer, child_type, varying_added); } } else { assert(buffer < NIR_MAX_XFB_BUFFERS); @@ -85,11 +121,9 @@ add_var_xfb_outputs(nir_xfb_info *xfb, uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; unsigned location_frac = var->data.location_frac; - nir_xfb_varying_info *varying = >varyings[xfb->varying_count++]; - varying->type = type; - varying->buffer = var->data.xfb_buffer; - varying->offset = *offset; - xfb->buffers[var->data.xfb_buffer].varying_count++; + if (!varying_added) { + add_var_xfb_varying(xfb, var, *offset, type); + } assert(attrib_slots <= 2); for (unsigned s = 0; s < attrib_slots; s++) { @@ -209,7 +243,7 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) for (unsigned i = 0; i < num_iterations; i++, buffer++) { unsigned offset = var->data.offset; -add_var_xfb_outputs(xfb, var, , , buffer, top_level_type); +add_var_xfb_outputs(xfb, var, , , buffer, top_level_type, false); } } } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 02/13] nir: don't assert when xfb_buffer/stride is present but not xfb_offset
In order to allow nir_gather_xfb_info to be used on OpenGL, specifically ARB_gl_spirv. So, from OpenGL 4.6 spec, section 11.1.2.1, "Output Variables": "outputs specifying both an *XfbBuffer* and an *Offset* are captured, while outputs not specifying both of these are not captured. Values are captured each time the shader writes to such a decorated object." This implies that are captured if both are present, and not if one of those are lacking. Technically, it doesn't explicitly point that having just one or the other is a mistake. In some cases, glslang is adding some extra XfbBuffer without XfbOffset around, and mentioning that technically that is not a bug (see issue#1526) And for the case of Vulkan, as the same glslang issue mentions, it is not clear if that should be a mistake or not. But even if it is a mistake, it is not really needed to be checked on the driver, and we can let the validation layers to check that. v2: simplify explicit_xfb_buffer and explicit_offset checks (Jason). Reviewed-by: Jason Ekstrand --- src/compiler/nir/nir_gather_xfb_info.c | 13 ++--- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index e282bba0081..f8d4cd833c7 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -107,11 +107,9 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) */ unsigned num_outputs = 0; nir_foreach_variable(var, >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); + if (var->data.explicit_xfb_buffer && + var->data.explicit_offset) { + num_outputs += glsl_count_attribute_slots(var->type, false); } } @@ -122,8 +120,9 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) /* Walk the list of outputs and add them to the array */ nir_foreach_variable(var, >outputs) { - if (var->data.explicit_xfb_buffer || - var->data.explicit_xfb_stride) { + if (var->data.explicit_xfb_buffer && + var->data.explicit_offset) { + unsigned location = var->data.location; unsigned offset = var->data.offset; add_var_xfb_outputs(xfb, var, , , var->type); -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 11/13] nir: adding varyings on nir_xfb_info and gather_info
In order to be used for OpenGL (right now for ARB_gl_spirv). This commit adds two new structures: * nir_xfb_varying_info: that identifies each individual varying. For each one, we need to know the type, buffer and xfb_offset * nir_xfb_buffer_info: as now for each buffer, in addition to the stride, we need to know how many varyings are assigned to it. At this point, the only case where num_outputs != num_varyings is with the case of doubles, that for dvec3/4 could require more than one output. There are more cases though, that will be handled on following patches. As it is somewhat more complex to know the number of varyings needed that the number of outputs, and num_varyings will be always less that num_outputs, we are using num_outputs as an approximation when allocating memory. This is debatable though. One alternative would be to allocate as needed, as the original ARB_gl_spirv custom xfb gathering pass was doing. --- src/compiler/nir/nir_gather_xfb_info.c | 27 +++--- src/compiler/nir/nir_xfb_info.h| 24 +++ 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index c46af311b20..e3c3376fb34 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -25,6 +25,17 @@ #include +static nir_xfb_info * +nir_gather_xfb_info_create(void *mem_ctx, uint16_t output_count, uint16_t varying_count) +{ + nir_xfb_info *xfb = rzalloc_size(mem_ctx, sizeof(nir_xfb_info)); + + xfb->varyings = rzalloc_size(mem_ctx, sizeof(nir_xfb_varying_info) * varying_count); + xfb->outputs = rzalloc_size(mem_ctx, sizeof(nir_xfb_output_info) * output_count); + + return xfb; +} + static void add_var_xfb_outputs(nir_xfb_info *xfb, nir_variable *var, @@ -48,11 +59,11 @@ add_var_xfb_outputs(nir_xfb_info *xfb, } else { assert(buffer < NIR_MAX_XFB_BUFFERS); if (xfb->buffers_written & (1 << buffer)) { - assert(xfb->strides[buffer] == var->data.xfb_stride); + assert(xfb->buffers[buffer].stride == var->data.xfb_stride); assert(xfb->buffer_to_stream[buffer] == var->data.stream); } else { xfb->buffers_written |= (1 << buffer); - xfb->strides[buffer] = var->data.xfb_stride; + xfb->buffers[buffer].stride = var->data.xfb_stride; xfb->buffer_to_stream[buffer] = var->data.stream; } @@ -74,6 +85,12 @@ add_var_xfb_outputs(nir_xfb_info *xfb, uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; unsigned location_frac = var->data.location_frac; + nir_xfb_varying_info *varying = >varyings[xfb->varying_count++]; + varying->type = type; + varying->buffer = var->data.xfb_buffer; + varying->offset = *offset; + xfb->buffers[var->data.xfb_buffer].varying_count++; + assert(attrib_slots <= 2); for (unsigned s = 0; s < attrib_slots; s++) { nir_xfb_output_info *output = >outputs[xfb->output_count++]; @@ -149,7 +166,11 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) if (num_outputs == 0) return NULL; - nir_xfb_info *xfb = rzalloc_size(mem_ctx, nir_xfb_info_size(num_outputs)); + /* It is complex to know how many varyings do we have beforehand. We use +* num_outputs as an approximation, as num_outputs should be bigger that +* num_varyings. +*/ + nir_xfb_info *xfb = nir_gather_xfb_info_create(mem_ctx, num_outputs, num_outputs); /* Walk the list of outputs and add them to the array */ nir_foreach_variable(var, >outputs) { diff --git a/src/compiler/nir/nir_xfb_info.h b/src/compiler/nir/nir_xfb_info.h index fef52ba96d8..71f4e87018c 100644 --- a/src/compiler/nir/nir_xfb_info.h +++ b/src/compiler/nir/nir_xfb_info.h @@ -29,6 +29,11 @@ #define NIR_MAX_XFB_BUFFERS 4 #define NIR_MAX_XFB_STREAMS 4 +typedef struct { + uint16_t stride; + uint16_t varying_count; +} nir_xfb_buffer_info; + typedef struct { uint8_t buffer; uint16_t offset; @@ -37,23 +42,26 @@ typedef struct { uint8_t component_offset; } nir_xfb_output_info; +typedef struct { + const struct glsl_type *type; + uint8_t buffer; + uint16_t offset; +} nir_xfb_varying_info; + typedef struct { uint8_t buffers_written; uint8_t streams_written; - uint16_t strides[NIR_MAX_XFB_BUFFERS]; + nir_xfb_buffer_info buffers[NIR_MAX_XFB_BUFFERS]; uint8_t buffer_to_stream[NIR_MAX_XFB_STREAMS]; + uint16_t varying_count; + nir_xfb_varying_info *varyings; + uint16_t output_count; - nir_xfb_output_info outputs[0]; + nir_xfb_output_info *outputs; } 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); -- 2.19.1
[Mesa-dev] [RFC PATCH 08/13] spirv/nir: interface_type should only be set for blocks, not any structs
Current code assumes that if the type is an struct it would behave as a block. That is not always the case (like xfb_offset/xfb_buffer assignment on arrays of structs vs arrays of blocks), so we need to differentiate. --- src/compiler/spirv/vtn_variables.c | 17 + 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index be3545aad47..6d7d5dfc691 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1677,9 +1677,12 @@ add_missing_member_locations(struct vtn_variable *var, * * “If the structure type is a Block but without a Location, then each * of its members must have a Location decoration.” + * */ - assert(var->base_location != -1 || - var->var->members[i].location != -1); + if (var->type->block) { + assert(var->base_location != -1 || +var->var->members[i].location != -1); + } /* From the Vulkan spec: * @@ -1692,8 +1695,12 @@ add_missing_member_locations(struct vtn_variable *var, else var->var->members[i].location = location; + /* Below we use type instead of interface_type, because interface_type + * is only available when it is a Block. This code also supports + * input/outputs that are just structs + */ const struct glsl_type *member_type = - glsl_get_struct_field(var->var->interface_type, i); + glsl_get_struct_field(glsl_without_array(var->type->type), i); location += glsl_count_attribute_slots(member_type, is_vertex_input); @@ -1862,9 +1869,11 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, interface_type = var->type->array_element; } - if (glsl_type_is_struct(interface_type->type)) { + if (interface_type->block) { var->var->interface_type = interface_type->type; + } + if (glsl_type_is_struct(interface_type->type)) { /* It's a struct. Set it up as per-member. */ var->var->num_members = glsl_get_length(interface_type->type); var->var->members = rzalloc_array(var->var, struct nir_variable_data, -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 10/13] nir/xfb: WIP: handle xfb buffer/offset rule for block arrays
From GLSL 4.60 spec, Section 4.4.2. Output Layout Qualifiers, subsection Transform Feedback Layout Qualifiers: "When a block is declared as an array, all members of block array-element 0 are captured, as previously described, by the declared or inherited xfb_buffer. Generally, an array of size N of blocks is captured by N consecutive buffers, with all members of block array-element E captured by buffer B, where B equals the declared or inherited xfb_buffer plus E" And although not explicitly mentioned, one conclusion for this paragraph would be that the xfb offset remain the same for the same member of each array-element. WIP: xfb arrays of blocks tests still not working properly due location overlapping. --- src/compiler/nir/nir_gather_xfb_info.c | 57 -- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index 6611691b686..c46af311b20 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -30,28 +30,30 @@ add_var_xfb_outputs(nir_xfb_info *xfb, nir_variable *var, unsigned *location, unsigned *offset, +unsigned buffer, 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); + add_var_xfb_outputs(xfb, var, location, offset, buffer, 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); + add_var_xfb_outputs(xfb, var, location, offset, buffer, 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); + assert(buffer < NIR_MAX_XFB_BUFFERS); + if (xfb->buffers_written & (1 << buffer)) { + assert(xfb->strides[buffer] == var->data.xfb_stride); + assert(xfb->buffer_to_stream[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; + xfb->buffers_written |= (1 << buffer); + xfb->strides[buffer] = var->data.xfb_stride; + xfb->buffer_to_stream[buffer] = var->data.stream; } assert(var->data.stream < NIR_MAX_XFB_STREAMS); @@ -76,7 +78,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, for (unsigned s = 0; s < attrib_slots; s++) { nir_xfb_output_info *output = >outputs[xfb->output_count++]; - output->buffer = var->data.xfb_buffer; + output->buffer = buffer; output->offset = *offset; output->location = *location; output->component_mask = (comp_mask >> (s * 4)) & 0xf; @@ -154,9 +156,40 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) if (var->data.explicit_xfb_buffer && var->data.explicit_offset) { + unsigned buffer = var->data.xfb_buffer; unsigned location = var->data.location; - unsigned offset = var->data.offset; - add_var_xfb_outputs(xfb, var, , , var->type); + + /* The last check is needed to distinguish a block array from a block + * that contains an array. That becomes messy due all the + * nir_split_per_members passes, as at this point we are not going to + * receive the original block array type, but splitted + */ + bool block_array = glsl_type_is_array(var->type) && +var->interface_type != NULL && +glsl_get_array_element(var->type) == var->interface_type; + + /* + * From GLSL 4.60 spec, Section 4.4.2. Output Layout Qualifiers, + * subsection Transform Feedback Layout Qualifiers: + * + * "When a block is declared as an array, all members of block + * array-element 0 are captured, as previously described, by the + * declared or inherited xfb_buffer. Generally, an array of size N + * of blocks is captured by N consecutive buffers, with all members + * of block array-element E captured by buffer B, where B equals the + * declared or inherited xfb_buffer plus E" + * +
[Mesa-dev] [RFC PATCH 09/13] nir/spirv: only expose interface type for arrays of interface blocks
In the same way that just a struct is not a interface block, and array of structs is not an array of interface blocks. At least at the NIR level. --- src/compiler/spirv/vtn_variables.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 6d7d5dfc691..a8f2fdfa534 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1860,7 +1860,7 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, */ struct vtn_type *interface_type = var->type; - if (!var->patch && glsl_type_is_array(var->type->type)) { + if (!var->patch && glsl_type_is_array(var->type->type) && var->type->array_element->block) { /* On Vulkan, Geometry shaders and some Tessellation, some inputs * come in per-vertex arrays, so we need to check for arrays. On * OpenGL we have the same, plus the possibility of user-defined -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 05/13] spirv/nir: fixing the xfb_offset for arrays of structs
GLSLang computes the xfb_offset for struct members. In fact, for basic structs, the xfb nir gathering pass expect those to be filled, as one struct variable is lowered to several nir variables, and those need to have the xfb offset already set. See [1]. But, as one existing comments at spirv to nir already points: "GLSLang really likes to place decorations in the most interior thing it possibly can. In particular, if you have a struct, it will place the patch decorations on the struct members" And that includes xfb offset. In fact, GLSLang not expose the xfb offset of the full struct, as it is properly assigned to the members, and it makes a lot of the internal checks (like offset overlapping) easier for them. I was not able to find a spec quote saying that is wrong, as all the individual members has the proper offset. This affects the case of variables that are array of structs, are they are exposed as just one nir variable output. So this commit resets the xfb_offset for the nir_variable if any of the members has a xfb_offset assigned. Rant: In general, the rules for xfb offset assignment on the spec are somewhat underspecified for the new ARB_gl_spirv/vulkan world, as it is not clear who is the responsible to do that (in opposite to the old GLSL world, where the answer is "always/everything should solved by the driver"). Ideally, it would be good if glslang does it, so the vulkan/opengl driver just need to get the info. Unfourtunately, there are cases, like arrays of structs where the driver still need to do the assignment. So perhaps in the end it should be the opposite, glslang (or any other frontend), just exposing the explicit info from the user, and let the driver do the individual assignments. Unfourtunately, with the current spec, there isn't anything preventing the frontend to do that, so we would need to be defensive, and cover all aspects. [1] https://github.com/KhronosGroup/glslang/pull/154 --- src/compiler/spirv/vtn_variables.c | 22 ++ 1 file changed, 22 insertions(+) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 13e8bf1fc3c..91e351187d2 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1718,6 +1718,26 @@ add_missing_member_locations(struct vtn_variable *var, } } +static void +vtn_fix_struct_array_xfb_offset(nir_variable *var) +{ + if (!glsl_type_is_array(var->type)) + return; + + const struct glsl_type *child_type = glsl_get_array_element(var->type); + + if (!glsl_type_is_struct(child_type)) + return; + + if (var->data.explicit_offset) + return; + + int offset = glsl_get_struct_field_offset(child_type, 0); + if (offset != -1) { + var->data.explicit_offset = 1; + var->data.offset = offset; + } +} static void vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, @@ -1882,6 +1902,8 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, vtn_foreach_decoration(b, vtn_value(b, interface_type->id, vtn_value_type_type), var_decoration_cb, var); + + vtn_fix_struct_array_xfb_offset(var->var); break; } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 00/13] Reusing nir xfb gathering for ARB_gl_spirv, wip array of blocks, plus other fixes
Hi Jason, as we were talking about arrays of blocks, xfb and other etc, I decided to clean up the code I have so far, and send a new RFC, so you know more or less what I have been doing. With this series most of the xfb tests I have wrotten pass so far (that includes arrays of structs, arrays of arrays of basic simples). The only pending thing is how to deal with arrays of blocks of input/output interface blocks, because as I told you privately, right now locations are overlapping. It is worth to note that due the same reason, a basic test using input/output interface blocks now are regressing. It worked before the block being splitted, and fails now. I assume that due the same reason. You can also find this series here: https://github.com/Igalia/mesa/tree/apinheiro/rfc2-xfb And the piglit series that adds xfb tests and a non-xfb array of blocks tests (that as I mentioned, regresses) here: https://github.com/Igalia/piglit/tree/apinheiro/xfb Alejandro Piñeiro (13): spirv/nir: update Xfb decoration comment nir: don't assert when xfb_buffer/stride is present but not xfb_offset nir: fix output offset compute for dvec3/4 nir: add component_offset at nir_xfb_info spirv/nir: fixing the xfb_offset for arrays of structs nir: fixing the xfb_offset for arrays of structs spirv/nir: use array_element as interface_type for any array spirv/nir: interface_type should only be set for blocks, not any structs nir/spirv: only expose interface type for arrays of interface blocks nir/xfb: WIP: handle xfb buffer/offset rule for block arrays nir: adding varyings on nir_xfb_info and gather_info nir/xfb_info: handle arrays and AoA of basic types nir/linker: use nir_gather_xfb_info src/compiler/glsl/gl_nir_link_xfb.c| 252 +++-- src/compiler/nir/nir_gather_xfb_info.c | 162 +--- src/compiler/nir/nir_xfb_info.h| 25 ++- src/compiler/spirv/spirv_to_nir.c | 2 +- src/compiler/spirv/vtn_variables.c | 68 --- 5 files changed, 270 insertions(+), 239 deletions(-) -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 04/13] nir: add component_offset at nir_xfb_info
Where component_offset here is the offset when accessing components of a packed variable. Or in other words, location_frac on nir.h. Different places of mesa use different names for it. Technically nir_xfb_info consumer can get the same from the component_mask, it seems somewhat forced to make it to compute it, instead of providing it. --- src/compiler/nir/nir_gather_xfb_info.c | 3 +++ src/compiler/nir/nir_xfb_info.h| 1 + 2 files changed, 4 insertions(+) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index f4f597da4f5..bf432583ddb 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -70,6 +70,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, assert(var->data.location_frac + comp_slots <= 8); uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; + unsigned location_frac = var->data.location_frac; assert(attrib_slots <= 2); for (unsigned s = 0; s < attrib_slots; s++) { @@ -79,6 +80,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, output->offset = *offset; output->location = *location; output->component_mask = (comp_mask >> (s * 4)) & 0xf; + output->component_offset = location_frac; (*location)++; /* attrib_slots would be only > 1 for doubles. On that case @@ -87,6 +89,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, */ assert(comp_slots % attrib_slots == 0); *offset += (comp_slots / attrib_slots) * 4; + location_frac = 0; } } } diff --git a/src/compiler/nir/nir_xfb_info.h b/src/compiler/nir/nir_xfb_info.h index 9b543df5f47..fef52ba96d8 100644 --- a/src/compiler/nir/nir_xfb_info.h +++ b/src/compiler/nir/nir_xfb_info.h @@ -34,6 +34,7 @@ typedef struct { uint16_t offset; uint8_t location; uint8_t component_mask; + uint8_t component_offset; } nir_xfb_output_info; typedef struct { -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 01/13] spirv/nir: update Xfb decoration comment
Now Vulkan radv driver, and ARB_gl_spirv implementation supports transform feedback. Having said so, those decorations are handled elsewhere. Reviewed-by: Jason Ekstrand --- src/compiler/spirv/spirv_to_nir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 4e1ffc3fcbe..d1a86210656 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -775,7 +775,7 @@ struct_member_decoration_cb(struct vtn_builder *b, case SpvDecorationXfbBuffer: case SpvDecorationXfbStride: - vtn_warn("Vulkan does not have transform feedback"); + /* Handled at vtn_variables.c, apply_var_decoration */ break; case SpvDecorationCPacked: -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 03/13] nir: fix output offset compute for dvec3/4
The offset compute was working fine for the case of attrib_slots=1, and updating the offset for the following varying. But in the case of attrib_slots=2 (so dvec3/4), we are basically splitting the comp_slots needed in two outputs. In that case we can't add to the offset the full size of the type. v2: added assert and some parenthesis to improve readability (Jason) Reviewed-by: Jason Ekstrand --- src/compiler/nir/nir_gather_xfb_info.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index f8d4cd833c7..f4f597da4f5 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -81,7 +81,12 @@ add_var_xfb_outputs(nir_xfb_info *xfb, output->component_mask = (comp_mask >> (s * 4)) & 0xf; (*location)++; - *offset += comp_slots * 4; + /* attrib_slots would be only > 1 for doubles. On that case + * comp_slots will be a multiple of 2, so the following doesn't need + * to use DIV_ROUND_UP or similar + */ + assert(comp_slots % attrib_slots == 0); + *offset += (comp_slots / attrib_slots) * 4; } } } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 07/13] spirv/nir: use array_element as interface_type for any array
This commit removes several of the checks when assigning the array_element as the interface_type. Reading the comment, and what commit bb04b84114d2780307f9cbd04447216c3f2d1c0c added on top, this is done conservatively, for only the builtin cases that makes sense at that moment. But even if those were true, that should be already validated on the SPIR-V shader. Additionally, it is not clear that user-defined array of input/output blocks are not allowed on Vulkan. And for sure, they will be allowed on OpenGL (via ARB_gl_spirv), so that method was too restrictive. --- src/compiler/spirv/vtn_variables.c | 29 + 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 91e351187d2..be3545aad47 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1664,24 +1664,6 @@ vtn_pointer_from_ssa(struct vtn_builder *b, nir_ssa_def *ssa, return ptr; } -static bool -is_per_vertex_inout(const struct vtn_variable *var, gl_shader_stage stage) -{ - if (var->patch || !glsl_type_is_array(var->type->type)) - return false; - - if (var->mode == vtn_variable_mode_input) { - return stage == MESA_SHADER_TESS_CTRL || - stage == MESA_SHADER_TESS_EVAL || - stage == MESA_SHADER_GEOMETRY; - } - - if (var->mode == vtn_variable_mode_output) - return stage == MESA_SHADER_TESS_CTRL; - - return false; -} - static void add_missing_member_locations(struct vtn_variable *var, bool is_vertex_input) @@ -1871,12 +1853,11 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, */ struct vtn_type *interface_type = var->type; - if (is_per_vertex_inout(var, b->shader->info.stage)) { - /* In Geometry shaders (and some tessellation), inputs come - * in per-vertex arrays. However, some builtins come in - * non-per-vertex, hence the need for the is_array check. In - * any case, there are no non-builtin arrays allowed so this - * check should be sufficient. + if (!var->patch && glsl_type_is_array(var->type->type)) { + /* On Vulkan, Geometry shaders and some Tessellation, some inputs + * come in per-vertex arrays, so we need to check for arrays. On + * OpenGL we have the same, plus the possibility of user-defined + * inout block arrays. */ interface_type = var->type->array_element; } -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 06/13] nir: fixing the xfb_offset for arrays of structs
Equivalent to previous patch (so comments applies), but implemented on a different place. We would need to chose in which one. --- src/compiler/nir/nir_gather_xfb_info.c | 23 +++ 1 file changed, 23 insertions(+) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index bf432583ddb..6611691b686 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -101,6 +101,27 @@ compare_xfb_output_offsets(const void *_a, const void *_b) return a->offset - b->offset; } +static void +fix_struct_array_xfb_offset(nir_variable *var) +{ + if (!glsl_type_is_array(var->type)) + return; + + const struct glsl_type *child_type = glsl_get_array_element(var->type); + + if (!glsl_type_is_struct(child_type)) + return; + + if (var->data.explicit_offset) + return; + + int offset = glsl_get_struct_field_offset(child_type, 0); + if (offset != -1) { + var->data.explicit_offset = 1; + var->data.offset = offset; + } +} + nir_xfb_info * nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) { @@ -115,6 +136,8 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) */ unsigned num_outputs = 0; nir_foreach_variable(var, >outputs) { + fix_struct_array_xfb_offset(var); + if (var->data.explicit_xfb_buffer && var->data.explicit_offset) { -- 2.19.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying
Hi Jason, just one thing here. Although I appreciate your interest to understand how varyings are enumerated, I think that we are diverting here, as in the end that would be something that I would need to solve. I just wanted to know for the way to go. The main question here is if we are really interested on adding such complexity on the general xfb gathering pass. This RFC was basically a way to show how much changes we would need, even for a incomplete solution Im not totally happy. So at this point, do you think that it is worth to add varying computation to the general pass in the name of code reuse, or should ARB_gl_spirv stick to their own gathering pass? On 10/11/18 12:13, Alejandro Piñeiro wrote: > On 09/11/18 16:58, Jason Ekstrand wrote: >> On November 9, 2018 06:39:25 Alejandro Piñeiro >> wrote: >>> On 08/11/18 23:14, Jason Ekstrand wrote: >>>> On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro >>>> mailto:apinhe...@igalia.com>> wrote: >>>> >>>> On OpenGL, a array of a simple type adds just one varying. So >>>> gl_transform_feedback_varying_info struct defined at mtypes.h >>>> includes >>>> the parameters Type (base_type) and Size (number of elements). >>>> >>>> This commit checks this when the recursive add_var_xfb_outputs call >>>> handles arrays, to ensure that just one is addded. >>>> >>>> RFC: Until this point, all changes were reasonable, but this >>>> change is >>>> (imho) ugly. My idea was introducing as less as possible changes on >>>> the code, specially on its logic/flow. But this commit is almost a >>>> hack. The ideal solution would be to change the focus of the >>>> recursive >>>> function, focusing on varyings, and at each varying, >>>> recursively add >>>> outputs. But that seems like an overkill for a pass that was >>>> originally intended for consumers only caring about the outputs. So >>>> perhaps ARB_gl_spirv should keep their own gathering pass, with >>>> vayings and outputs, and let this one untouched for those that only >>>> care on outputs. >>>> --- >>>> src/compiler/nir/nir_gather_xfb_info.c | 52 >>>> -- >>>> 1 file changed, 43 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/src/compiler/nir/nir_gather_xfb_info.c >>>> b/src/compiler/nir/nir_gather_xfb_info.c >>>> index 948b802a815..cb0e2724cab 100644 >>>> --- a/src/compiler/nir/nir_gather_xfb_info.c >>>> +++ b/src/compiler/nir/nir_gather_xfb_info.c >>>> @@ -36,23 +36,59 @@ nir_gather_xfb_info_create(void *mem_ctx, >>>> uint16_t output_count, uint16_t varyin >>>> return xfb; >>>> } >>>> >>>> +static bool >>>> +glsl_type_is_leaf(const struct glsl_type *type) >>>> +{ >>>> + if (glsl_type_is_struct(type) || >>>> + (glsl_type_is_array(type) && >>>> + (glsl_type_is_array(glsl_get_array_element(type)) || >>>> + glsl_type_is_struct(glsl_get_array_element(type) { >>>> >>>> >>>> I'm trying to understand exactly what this means. From what you >>>> wrote here it looks like the following are all one varying: >>>> >>>> float var[3]; >>>> vec2 var[3]; >>>> mat4 var[3]; >>> >>> Yes, GLSL returns one varying per each one (Size 3). >> >> Just to be clear, a matrix it array of matrices is one varying? > > Yep, and being more clear, for this shader: > #version 150 > #extension GL_ARB_enhanced_layouts: require > > layout(xfb_offset = 0) out mat4 var[3]; > > void main() { > mat4 m4; > > gl_Position = vec4(0.0); > > var[0] = m4; > } > > We get the following when we dump gl_program::LinkedTransformFeedback, > that is a struct gl_transform_feedback_info defined at mtypes.h: > > [gl_transform_feedback_info] > NumOuputs = 12, (OutputRegister, OutputBuffer, NumComponents, > StreamId, DstOffset, ComponentOffset) > 0:(31, 0, 4, 0, 0, 0) > 1:(32, 0, 4, 0, 4, 0) > 2:(33, 0, 4, 0, 8, 0) > 3:(34, 0, 4, 0, 12, 0) > 4:(35, 0, 4, 0, 16, 0) > 5:(36, 0, 4, 0, 20, 0) > 6:(37, 0, 4, 0, 24, 0) > 7:(
Re: [Mesa-dev] [PATCH 2/2] i965: Do NIR shader cloning in the caller.
I was tempted to suggest to add a comment somewhere mentioning this policy change, but there are so many functions that Im not sure what would be that somewhere. Ramblings apart: Reviewed-by: Alejandro Piñeiro On 10/11/18 09:17, Kenneth Graunke wrote: > This moves nir_shader_clone() to the driver-specific compile function, > rather than the shared src/intel/compiler code. This allows i965 to do > key-specific passes before calling brw_compile_*. Vulkan should not > need this cloning as it doesn't compile multiple variants. > > We do need to continue cloning in the compute shader code because we > lower various things in NIR based on the SIMD width. > --- > src/intel/compiler/brw_compiler.h | 10 +- > src/intel/compiler/brw_fs.cpp | 3 +-- > src/intel/compiler/brw_shader.cpp | 3 +-- > src/intel/compiler/brw_vec4.cpp| 3 +-- > src/intel/compiler/brw_vec4_gs_visitor.cpp | 3 +-- > src/intel/compiler/brw_vec4_tcs.cpp| 3 +-- > src/mesa/drivers/dri/i965/brw_cs.c | 2 +- > src/mesa/drivers/dri/i965/brw_gs.c | 2 +- > src/mesa/drivers/dri/i965/brw_tcs.c| 2 +- > src/mesa/drivers/dri/i965/brw_tes.c| 3 ++- > src/mesa/drivers/dri/i965/brw_vs.c | 2 +- > src/mesa/drivers/dri/i965/brw_wm.c | 2 +- > 12 files changed, 17 insertions(+), 21 deletions(-) > > diff --git a/src/intel/compiler/brw_compiler.h > b/src/intel/compiler/brw_compiler.h > index d8c9499065f..1daf935d7fe 100644 > --- a/src/intel/compiler/brw_compiler.h > +++ b/src/intel/compiler/brw_compiler.h > @@ -1238,7 +1238,7 @@ brw_compile_vs(const struct brw_compiler *compiler, > void *log_data, > void *mem_ctx, > const struct brw_vs_prog_key *key, > struct brw_vs_prog_data *prog_data, > - const struct nir_shader *shader, > + struct nir_shader *shader, > int shader_time_index, > char **error_str); > > @@ -1253,7 +1253,7 @@ brw_compile_tcs(const struct brw_compiler *compiler, > void *mem_ctx, > const struct brw_tcs_prog_key *key, > struct brw_tcs_prog_data *prog_data, > -const struct nir_shader *nir, > +struct nir_shader *nir, > int shader_time_index, > char **error_str); > > @@ -1268,7 +1268,7 @@ brw_compile_tes(const struct brw_compiler *compiler, > void *log_data, > const struct brw_tes_prog_key *key, > const struct brw_vue_map *input_vue_map, > struct brw_tes_prog_data *prog_data, > -const struct nir_shader *shader, > +struct nir_shader *shader, > struct gl_program *prog, > int shader_time_index, > char **error_str); > @@ -1283,7 +1283,7 @@ brw_compile_gs(const struct brw_compiler *compiler, > void *log_data, > void *mem_ctx, > const struct brw_gs_prog_key *key, > struct brw_gs_prog_data *prog_data, > - const struct nir_shader *shader, > + struct nir_shader *shader, > struct gl_program *prog, > int shader_time_index, > char **error_str); > @@ -1330,7 +1330,7 @@ brw_compile_fs(const struct brw_compiler *compiler, > void *log_data, > void *mem_ctx, > const struct brw_wm_prog_key *key, > struct brw_wm_prog_data *prog_data, > - const struct nir_shader *shader, > + struct nir_shader *shader, > struct gl_program *prog, > int shader_time_index8, > int shader_time_index16, > diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp > index 3e083723471..aa29c8a4deb 100644 > --- a/src/intel/compiler/brw_fs.cpp > +++ b/src/intel/compiler/brw_fs.cpp > @@ -7119,7 +7119,7 @@ brw_compile_fs(const struct brw_compiler *compiler, > void *log_data, > void *mem_ctx, > const struct brw_wm_prog_key *key, > struct brw_wm_prog_data *prog_data, > - const nir_shader *src_shader, > + nir_shader *shader, > struct gl_program *prog, > int shader_time_index8, int shader_time_index16, > int shader_time_index32, bool allow_spilling, > @@ -7128,7 +7128,6 @@ brw_compile_fs(const struct brw_compiler *compiler, > void *log_data, > { > const struct gen_device_info *devinfo = compiler->devinfo; > > - nir_shader *shader = nir_shad
Re: [Mesa-dev] [PATCH 1/2] i965: Use a 'nir' temporary rather than poking at brw_program
Reviewed-by: Alejandro Piñeiro On 10/11/18 09:17, Kenneth Graunke wrote: > It's shorter and will also be useful when I adjust cloning soon. > --- > src/mesa/drivers/dri/i965/brw_cs.c | 6 +++--- > src/mesa/drivers/dri/i965/brw_gs.c | 11 ++- > src/mesa/drivers/dri/i965/brw_tcs.c | 2 +- > src/mesa/drivers/dri/i965/brw_tes.c | 2 +- > src/mesa/drivers/dri/i965/brw_vs.c | 15 --- > src/mesa/drivers/dri/i965/brw_wm.c | 11 ++- > 6 files changed, 25 insertions(+), 22 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_cs.c > b/src/mesa/drivers/dri/i965/brw_cs.c > index 498c80d46a5..3ae54830f78 100644 > --- a/src/mesa/drivers/dri/i965/brw_cs.c > +++ b/src/mesa/drivers/dri/i965/brw_cs.c > @@ -58,6 +58,7 @@ brw_codegen_cs_prog(struct brw_context *brw, > struct brw_cs_prog_data prog_data; > bool start_busy = false; > double start_time = 0; > + nir_shader *nir = cp->program.nir; > > memset(_data, 0, sizeof(prog_data)); > > @@ -76,7 +77,7 @@ brw_codegen_cs_prog(struct brw_context *brw, > > assign_cs_binding_table_offsets(devinfo, >program, _data); > > - brw_nir_setup_glsl_uniforms(mem_ctx, cp->program.nir, > + brw_nir_setup_glsl_uniforms(mem_ctx, nir, > >program, _data.base, true); > > if (unlikely(brw->perf_debug)) { > @@ -91,8 +92,7 @@ brw_codegen_cs_prog(struct brw_context *brw, > > char *error_str; > program = brw_compile_cs(brw->screen->compiler, brw, mem_ctx, key, > -_data, cp->program.nir, st_index, > -_str); > +_data, nir, st_index, _str); > if (program == NULL) { >cp->program.sh.data->LinkStatus = LINKING_FAILURE; >ralloc_strcat(>program.sh.data->InfoLog, error_str); > diff --git a/src/mesa/drivers/dri/i965/brw_gs.c > b/src/mesa/drivers/dri/i965/brw_gs.c > index 7263f6351e9..55c2923bded 100644 > --- a/src/mesa/drivers/dri/i965/brw_gs.c > +++ b/src/mesa/drivers/dri/i965/brw_gs.c > @@ -89,15 +89,17 @@ brw_codegen_gs_prog(struct brw_context *brw, > > void *mem_ctx = ralloc_context(NULL); > > + nir_shader *nir = gp->program.nir; > + > assign_gs_binding_table_offsets(devinfo, >program, _data); > > - brw_nir_setup_glsl_uniforms(mem_ctx, gp->program.nir, >program, > + brw_nir_setup_glsl_uniforms(mem_ctx, nir, >program, > _data.base.base, > compiler->scalar_stage[MESA_SHADER_GEOMETRY]); > - brw_nir_analyze_ubo_ranges(compiler, gp->program.nir, NULL, > + brw_nir_analyze_ubo_ranges(compiler, nir, NULL, >prog_data.base.base.ubo_ranges); > > - uint64_t outputs_written = gp->program.nir->info.outputs_written; > + uint64_t outputs_written = nir->info.outputs_written; > > brw_compute_vue_map(devinfo, > _data.base.vue_map, outputs_written, > @@ -115,8 +117,7 @@ brw_codegen_gs_prog(struct brw_context *brw, > char *error_str; > const unsigned *program = >brw_compile_gs(brw->screen->compiler, brw, mem_ctx, key, > - _data, gp->program.nir, >program, > - st_index, _str); > + _data, nir, >program, st_index, _str); > if (program == NULL) { >ralloc_strcat(>program.sh.data->InfoLog, error_str); >_mesa_problem(NULL, "Failed to compile geometry shader: %s\n", > error_str); > diff --git a/src/mesa/drivers/dri/i965/brw_tcs.c > b/src/mesa/drivers/dri/i965/brw_tcs.c > index 17f4130c095..6e60a44fc10 100644 > --- a/src/mesa/drivers/dri/i965/brw_tcs.c > +++ b/src/mesa/drivers/dri/i965/brw_tcs.c > @@ -100,7 +100,7 @@ brw_codegen_tcs_prog(struct brw_context *brw, struct > brw_program *tcp, >brw_nir_setup_glsl_uniforms(mem_ctx, nir, >program, >_data.base.base, > > compiler->scalar_stage[MESA_SHADER_TESS_CTRL]); > - brw_nir_analyze_ubo_ranges(compiler, tcp->program.nir, NULL, > + brw_nir_analyze_ubo_ranges(compiler, nir, NULL, > prog_data.base.base.ubo_ranges); > } else { >/* Upload the Patch URB Header as the first two uniforms. > diff --git a/src/mesa/drivers/dri/i965/brw_tes.c > b/src/mesa/drivers/dri/i965/brw_tes.c > index b3220a94741..8f8f68530b7 100644 > --- a/src/mesa/drivers/dri/i965/brw_tes.c > +++ b/src/mesa/drivers/dri/i965/brw_tes.c > @@ -85,7 +85,7 @@ brw_codegen_tes_prog(struct brw_context *brw, > brw_
Re: [Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying
On 09/11/18 16:58, Jason Ekstrand wrote: > On November 9, 2018 06:39:25 Alejandro Piñeiro > wrote: >> On 08/11/18 23:14, Jason Ekstrand wrote: >>> On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro >>> mailto:apinhe...@igalia.com>> wrote: >>> >>> On OpenGL, a array of a simple type adds just one varying. So >>> gl_transform_feedback_varying_info struct defined at mtypes.h >>> includes >>> the parameters Type (base_type) and Size (number of elements). >>> >>> This commit checks this when the recursive add_var_xfb_outputs call >>> handles arrays, to ensure that just one is addded. >>> >>> RFC: Until this point, all changes were reasonable, but this >>> change is >>> (imho) ugly. My idea was introducing as less as possible changes on >>> the code, specially on its logic/flow. But this commit is almost a >>> hack. The ideal solution would be to change the focus of the >>> recursive >>> function, focusing on varyings, and at each varying, recursively add >>> outputs. But that seems like an overkill for a pass that was >>> originally intended for consumers only caring about the outputs. So >>> perhaps ARB_gl_spirv should keep their own gathering pass, with >>> vayings and outputs, and let this one untouched for those that only >>> care on outputs. >>> --- >>> src/compiler/nir/nir_gather_xfb_info.c | 52 >>> -- >>> 1 file changed, 43 insertions(+), 9 deletions(-) >>> >>> diff --git a/src/compiler/nir/nir_gather_xfb_info.c >>> b/src/compiler/nir/nir_gather_xfb_info.c >>> index 948b802a815..cb0e2724cab 100644 >>> --- a/src/compiler/nir/nir_gather_xfb_info.c >>> +++ b/src/compiler/nir/nir_gather_xfb_info.c >>> @@ -36,23 +36,59 @@ nir_gather_xfb_info_create(void *mem_ctx, >>> uint16_t output_count, uint16_t varyin >>> return xfb; >>> } >>> >>> +static bool >>> +glsl_type_is_leaf(const struct glsl_type *type) >>> +{ >>> + if (glsl_type_is_struct(type) || >>> + (glsl_type_is_array(type) && >>> + (glsl_type_is_array(glsl_get_array_element(type)) || >>> + glsl_type_is_struct(glsl_get_array_element(type) { >>> >>> >>> I'm trying to understand exactly what this means. From what you >>> wrote here it looks like the following are all one varying: >>> >>> float var[3]; >>> vec2 var[3]; >>> mat4 var[3]; >> >> Yes, GLSL returns one varying per each one (Size 3). > > Just to be clear, a matrix it array of matrices is one varying? Yep, and being more clear, for this shader: #version 150 #extension GL_ARB_enhanced_layouts: require layout(xfb_offset = 0) out mat4 var[3]; void main() { mat4 m4; gl_Position = vec4(0.0); var[0] = m4; } We get the following when we dump gl_program::LinkedTransformFeedback, that is a struct gl_transform_feedback_info defined at mtypes.h: [gl_transform_feedback_info] NumOuputs = 12, (OutputRegister, OutputBuffer, NumComponents, StreamId, DstOffset, ComponentOffset) 0:(31, 0, 4, 0, 0, 0) 1:(32, 0, 4, 0, 4, 0) 2:(33, 0, 4, 0, 8, 0) 3:(34, 0, 4, 0, 12, 0) 4:(35, 0, 4, 0, 16, 0) 5:(36, 0, 4, 0, 20, 0) 6:(37, 0, 4, 0, 24, 0) 7:(38, 0, 4, 0, 28, 0) 8:(39, 0, 4, 0, 32, 0) 9:(40, 0, 4, 0, 36, 0) 10:(41, 0, 4, 0, 40, 0) 11:(42, 0, 4, 0, 44, 0) NumVarying=1, (Offset, Type, BufferIndex, Size, Name) 0:( 0, GL_FLOAT_MAT4, 0, 3, var) ActiveBuffers=1, (Binding, NumVaryings, Stride, Stream): 0:( 0, 1, 192, 0) FWIW, in some cases we are also getting a slightly different amount of Outputs. But Im personally not really worried about that as far as it keeps working. The number of varyings is somewhat different as it is exposed through the program interface queries, so (I assume) it should be consistent. > >> >>> >>> but the following are not >>> >>> struct S { >>> float f; >>> vec4 v; >>> }; >>> >>> S var[3]; >> >>> float var[3][5]; >> >> I guess that you are asking for thos two cases because this code is >> not handling it properly. You are right. For the array of structs, >> our code is crashing. For the array of
[Mesa-dev] [PATCH v2] nir: fix output offset compute for dvec3/4
The offset compute was working fine for the case of attrib_slots=1, and updating the offset for the following varying. But in the case of attrib_slots=2 (so dvec3/4), we are basically splitting the comp_slots needed in two outputs. In that case we can't add to the offset the full size of the type. v2: added assert and some parenthesis to improve readability (Jason) --- src/compiler/nir/nir_gather_xfb_info.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index f8d4cd833c7..f4f597da4f5 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -81,7 +81,12 @@ add_var_xfb_outputs(nir_xfb_info *xfb, output->component_mask = (comp_mask >> (s * 4)) & 0xf; (*location)++; - *offset += comp_slots * 4; + /* attrib_slots would be only > 1 for doubles. On that case + * comp_slots will be a multiple of 2, so the following doesn't need + * to use DIV_ROUND_UP or similar + */ + assert(comp_slots % attrib_slots == 0); + *offset += (comp_slots / attrib_slots) * 4; } } } -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/7] nir: don't assert when xfb_buffer/stride is present but not xfb_offset
On 08/11/18 22:42, Jason Ekstrand wrote: > On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <mailto:apinhe...@igalia.com>> wrote: > > In order to allow nir_gather_xfb_info to be used on OpenGL, > specifically ARB_gl_spirv. > > So, from OpenGL 4.6 spec, section 11.1.2.1, "Output Variables": > > "outputs specifying both an *XfbBuffer* and an *Offset* are > captured, while outputs not specifying both of these are not > captured. Values are captured each time the shader writes to such > a decorated object." > > This implies that are captured if both are present, and not if one of > those are lacking. Technically, it doesn't explicitly point that > having just one or the other is a mistake. In some cases, glslang is > adding some extra XfbBuffer without XfbOffset around, and mentioning > that technically that is not a bug (see issue#1526) > > And for the case of Vulkan, as the same glslang issue mentions, it is > not clear if that should be a mistake or not. But even if it is a > mistake, it is not really needed to be checked on the driver, and we > can let the validation layers to check that. > --- > src/compiler/nir/nir_gather_xfb_info.c | 23 --- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/src/compiler/nir/nir_gather_xfb_info.c > b/src/compiler/nir/nir_gather_xfb_info.c > index e282bba0081..f5d831c6567 100644 > --- a/src/compiler/nir/nir_gather_xfb_info.c > +++ b/src/compiler/nir/nir_gather_xfb_info.c > @@ -109,9 +109,16 @@ nir_gather_xfb_info(const nir_shader *shader, > void *mem_ctx) > nir_foreach_variable(var, >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); > + > + /* OpenGL points that both are needed to capture the > output, but > + * doesn't direcly imply that it is a mistake having one > but not the > + * other. > + */ > + if (!var->data.explicit_xfb_buffer || > + !var->data.explicit_offset) { > > > Why not just change the check above to "var->data.explicit_xfb_buffer > && var->data.explicit_offset" and not bother with two checks? True. Change done locally. > > > + continue; > + } > + > num_outputs += glsl_count_attribute_slots(var->type, false); > } > } > @@ -124,6 +131,16 @@ nir_gather_xfb_info(const nir_shader *shader, > void *mem_ctx) > nir_foreach_variable(var, >outputs) { > if (var->data.explicit_xfb_buffer || > var->data.explicit_xfb_stride) { > + > + /* OpenGL points that both are needed to capture the > output, but > + * doesn't direcly imply that it is a mistake having one > but not the > + * other. > + */ > + if (!var->data.explicit_xfb_buffer || > + !var->data.explicit_offset) { > > > Same here. > > > + continue; > + } > + > unsigned location = var->data.location; > unsigned offset = var->data.offset; > add_var_xfb_outputs(xfb, var, , , > var->type); > -- > 2.14.1 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying
On 08/11/18 23:14, Jason Ekstrand wrote: > On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <mailto:apinhe...@igalia.com>> wrote: > > On OpenGL, a array of a simple type adds just one varying. So > gl_transform_feedback_varying_info struct defined at mtypes.h includes > the parameters Type (base_type) and Size (number of elements). > > This commit checks this when the recursive add_var_xfb_outputs call > handles arrays, to ensure that just one is addded. > > RFC: Until this point, all changes were reasonable, but this change is > (imho) ugly. My idea was introducing as less as possible changes on > the code, specially on its logic/flow. But this commit is almost a > hack. The ideal solution would be to change the focus of the recursive > function, focusing on varyings, and at each varying, recursively add > outputs. But that seems like an overkill for a pass that was > originally intended for consumers only caring about the outputs. So > perhaps ARB_gl_spirv should keep their own gathering pass, with > vayings and outputs, and let this one untouched for those that only > care on outputs. > --- > src/compiler/nir/nir_gather_xfb_info.c | 52 > -- > 1 file changed, 43 insertions(+), 9 deletions(-) > > diff --git a/src/compiler/nir/nir_gather_xfb_info.c > b/src/compiler/nir/nir_gather_xfb_info.c > index 948b802a815..cb0e2724cab 100644 > --- a/src/compiler/nir/nir_gather_xfb_info.c > +++ b/src/compiler/nir/nir_gather_xfb_info.c > @@ -36,23 +36,59 @@ nir_gather_xfb_info_create(void *mem_ctx, > uint16_t output_count, uint16_t varyin > return xfb; > } > > +static bool > +glsl_type_is_leaf(const struct glsl_type *type) > +{ > + if (glsl_type_is_struct(type) || > + (glsl_type_is_array(type) && > + (glsl_type_is_array(glsl_get_array_element(type)) || > + glsl_type_is_struct(glsl_get_array_element(type) { > > > I'm trying to understand exactly what this means. From what you wrote > here it looks like the following are all one varying: > > float var[3]; > vec2 var[3]; > mat4 var[3]; Yes, GLSL returns one varying per each one (Size 3). > > but the following are not > > struct S { > float f; > vec4 v; > }; > > S var[3]; > float var[3][5]; I guess that you are asking for thos two cases because this code is not handling it properly. You are right. For the array of structs, our code is crashing. For the array of arrays, it is enumerating four varyings. One with three GL_FLOAT components, and three with five GL_FLOAT components, instead of just three varyings with five components. In my defense, I already mentioned that it was wip code, but preferred to agree on the way to go before keep working on it. For the GLSL case, the array of struct returns 6 varyings. And funny thing, for the array of arrays, GLSL is handling the situation even worse. It returns the following link error: "Failed to link: error: Transform feedback varying var[0] undeclared." Just a quick skim on the spec, I didn't see anything preventing using aoa with transform feedback varyings, so I guess that this is a bug due all the rules OpenGL has in relation with variable names. > > Is this correct? Yes. FWIW, I will give you another two examples, from the tests Im using as reference. ## example 1 ## struct Array { float x2_out; }; struct AoA { Array x2_Array[2]; }; struct S { float x1_out; AoA x2_AoA[2]; float x3_out; }; layout(xfb_offset = 0) out S s1; layout(xfb_offset = 0, xfb_buffer = 2) out struct S2 { float y1_out; vec4 y2_out; } s2; GLSL returns the following varyings (on ARB_gl_spirv we target to get the same, although without the names) NumVarying=8, (Offset, Type, BufferIndex, Size, Name) 0:( 0, GL_FLOAT, 0, 1, s1.x1_out) 1:( 4, GL_FLOAT, 0, 1, s1.x2_AoA[0].x2_Array[0].x2_out) 2:( 8, GL_FLOAT, 0, 1, s1.x2_AoA[0].x2_Array[1].x2_out) 3:(12, GL_FLOAT, 0, 1, s1.x2_AoA[1].x2_Array[0].x2_out) 4:(16, GL_FLOAT, 0, 1, s1.x2_AoA[1].x2_Array[1].x2_out) 5:(20, GL_FLOAT, 0, 1, s1.x3_out) 6:( 0, GL_FLOAT, 1, 1, s2.y1_out) 7:( 4, GL_FLOAT_VEC4, 1, 1, s2.y2_out) ## example 2 ## layout(xfb_offset = 0) out float x1_out; layout(xfb_offset = 4) out float x2_out[2]; layout(xfb_offset = 12) out vec3 x3_out; layout(xfb_buffer = 2) out; layout(xfb_offset = 0, xfb_buffer = 2) out float y1_out; layout(xfb_offset = 4) out vec4 y2_out GLSL returns the following varyings (on ARB_gl_spirv we target to get the sam
[Mesa-dev] [PATCH 3/7] nir: fix output offset compute for dvec3/4
The offset compute was working fine for the case of attrib_slots=1, and updating the offset for the following varying. But in the case of attrib_slots=2 (so dvec3/4), we are basically splitting the comp_slots needed in two outputs. In that case we can't add to the offset the full size of the type. --- src/compiler/nir/nir_gather_xfb_info.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index f5d831c6567..01fc2b26624 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -81,7 +81,11 @@ add_var_xfb_outputs(nir_xfb_info *xfb, output->component_mask = (comp_mask >> (s * 4)) & 0xf; (*location)++; - *offset += comp_slots * 4; + /* attrib_slots would be only > 1 for doubles. On that case + * comp_slots will be a multiple of 2, so the following doesn't need + * to use DIV_ROUND_UP or similar + */ + *offset += comp_slots / attrib_slots * 4; } } } -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 7/7] nir/linker: use nir_gather_xfb_info
Instead of a custom ARB_gl_spirv xfb gather info pass. In fact, this is not only about reusing code, but the current custom code was not handling properly how many varyings are enumerated from some complex types. So this change is also about fixing some corner cases. --- src/compiler/glsl/gl_nir_link_xfb.c | 252 +++- 1 file changed, 72 insertions(+), 180 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_xfb.c b/src/compiler/glsl/gl_nir_link_xfb.c index bcef1e1863d..b294a4885b0 100644 --- a/src/compiler/glsl/gl_nir_link_xfb.c +++ b/src/compiler/glsl/gl_nir_link_xfb.c @@ -22,8 +22,8 @@ */ #include "nir.h" +#include "nir_xfb_info.h" #include "gl_nir_linker.h" -#include "ir_uniform.h" /* for gl_uniform_storage */ #include "linker_util.h" #include "main/context.h" @@ -34,158 +34,13 @@ * particularities. */ -struct active_xfb_buffer { - GLuint stride; - GLuint num_varyings; -}; - -struct active_xfb_varyings { - unsigned num_varyings; - unsigned num_outputs; - unsigned buffer_size; - struct nir_variable **varyings; - struct active_xfb_buffer buffers[MAX_FEEDBACK_BUFFERS]; -}; - -static unsigned -get_num_outputs(nir_variable *var) -{ - return glsl_count_attribute_slots(var->type, - false /* is_vertex_input */); -} - -static void -add_xfb_varying(struct active_xfb_varyings *active_varyings, -nir_variable *var) -{ - if (active_varyings->num_varyings >= active_varyings->buffer_size) { - if (active_varyings->buffer_size == 0) - active_varyings->buffer_size = 1; - else - active_varyings->buffer_size *= 2; - - active_varyings->varyings = realloc(active_varyings->varyings, - sizeof(nir_variable*) * - active_varyings->buffer_size); - } - - active_varyings->varyings[active_varyings->num_varyings++] = var; - - active_varyings->num_outputs += get_num_outputs(var); -} - static int -cmp_xfb_offset(const void *x_generic, const void *y_generic) -{ - const nir_variable *const *x = x_generic; - const nir_variable *const *y = y_generic; - - if ((*x)->data.xfb_buffer != (*y)->data.xfb_buffer) - return (*x)->data.xfb_buffer - (*y)->data.xfb_buffer; - return (*x)->data.offset - (*y)->data.offset; -} - -static void -get_active_xfb_varyings(struct gl_shader_program *prog, -struct active_xfb_varyings *active_varyings) -{ - for (unsigned i = 0; i < MESA_SHADER_STAGES; ++i) { - struct gl_linked_shader *sh = prog->_LinkedShaders[i]; - if (sh == NULL) - continue; - - nir_shader *nir = sh->Program->nir; - - nir_foreach_variable(var, >outputs) { - if (var->data.explicit_xfb_buffer && - var->data.explicit_xfb_stride) { -assert(var->data.xfb_buffer < MAX_FEEDBACK_BUFFERS); -active_varyings->buffers[var->data.xfb_buffer].stride = - var->data.xfb_stride; - } - - if (!var->data.explicit_xfb_buffer || - !var->data.explicit_offset) -continue; - - active_varyings->buffers[var->data.xfb_buffer].num_varyings++; - - add_xfb_varying(active_varyings, var); - } - } - - /* The xfb_offset qualifier does not have to be used in increasing order -* however some drivers expect to receive the list of transform feedback -* declarations in order so sort it now for convenience. -*/ - qsort(active_varyings->varyings, - active_varyings->num_varyings, - sizeof(*active_varyings->varyings), - cmp_xfb_offset); -} - -static unsigned -add_varying_outputs(nir_variable *var, -const struct glsl_type *type, -unsigned location_offset, -unsigned dest_offset, -struct gl_transform_feedback_output *output) +count_bits(uint8_t mask) { - unsigned num_outputs = 0; - - 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); - unsigned component_slots = glsl_get_component_slots(child_type); - - for (unsigned i = 0; i < length; i++) { - unsigned child_outputs = add_varying_outputs(var, - child_type, - location_offset, - dest_offset, - output + num_outputs); - num_outputs += child_outputs; - location_offset += child_outputs; - dest_offset += component_slots; - } - } 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 =
[Mesa-dev] [PATCH 4/7] nir: add component_offset at nir_xfb_info
Where component_offset here is the offset when accessing components of a packed variable. Or in other words, location_frac on nir.h. Different places of mesa use different names for it. Technically nir_xfb_info consumer can get the same from the component_mask, it seems somewhat forced to make it to compute it, instead of providing it. --- src/compiler/nir/nir_gather_xfb_info.c | 3 +++ src/compiler/nir/nir_xfb_info.h| 1 + 2 files changed, 4 insertions(+) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index 01fc2b26624..cd3afa32661 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -70,6 +70,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, assert(var->data.location_frac + comp_slots <= 8); uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; + unsigned location_frac = var->data.location_frac; assert(attrib_slots <= 2); for (unsigned s = 0; s < attrib_slots; s++) { @@ -79,6 +80,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, output->offset = *offset; output->location = *location; output->component_mask = (comp_mask >> (s * 4)) & 0xf; + output->component_offset = location_frac; (*location)++; /* attrib_slots would be only > 1 for doubles. On that case @@ -86,6 +88,7 @@ add_var_xfb_outputs(nir_xfb_info *xfb, * to use DIV_ROUND_UP or similar */ *offset += comp_slots / attrib_slots * 4; + location_frac = 0; } } } diff --git a/src/compiler/nir/nir_xfb_info.h b/src/compiler/nir/nir_xfb_info.h index 9b543df5f47..fef52ba96d8 100644 --- a/src/compiler/nir/nir_xfb_info.h +++ b/src/compiler/nir/nir_xfb_info.h @@ -34,6 +34,7 @@ typedef struct { uint16_t offset; uint8_t location; uint8_t component_mask; + uint8_t component_offset; } nir_xfb_output_info; typedef struct { -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/7] spirv/nir: update Xfb decoration comment
Although it is true that Vulkan doesn't support transform feedback yet, spirv to nir is handling it due ARB_gl_spirv support. Having said so, those decorations are handled elsewhere. --- src/compiler/spirv/spirv_to_nir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 96ff09c3659..140e98eba27 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -774,7 +774,7 @@ struct_member_decoration_cb(struct vtn_builder *b, case SpvDecorationXfbBuffer: case SpvDecorationXfbStride: - vtn_warn("Vulkan does not have transform feedback"); + /* Handled at vtn_variables.c, apply_var_decoration */ break; case SpvDecorationCPacked: -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 5/7] RFC: nir: adding varyings on nir_xfb_info and gather_info
In order to be used for OpenGL (right now for ARB_gl_spirv). This commit adds two new structures: * nir_xfb_varying_info: that identifies each individual varying. For each one, we need to know the type, buffer and xfb_offset * nir_xfb_buffer_info: as now for each buffer, in addition to the stride, we need to know how many varyings are assigned to it. At this point, the only case where num_outputs!=num_varyings is with the case of doubles, that for dvec3/4 could require more than one output. There are more cases though, that will be handled on following patches. RFC: Also, as it is somewhat more complex to know the number of varyings needed that the number of outputs, and num_varyings will be always less that num_outputs, we are using num_outputs as an approximation when allocating memory. This is debatable though. --- src/compiler/nir/nir_gather_xfb_info.c | 27 --- src/compiler/nir/nir_xfb_info.h| 24 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index cd3afa32661..948b802a815 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -25,6 +25,17 @@ #include +static nir_xfb_info * +nir_gather_xfb_info_create(void *mem_ctx, uint16_t output_count, uint16_t varying_count) +{ + nir_xfb_info *xfb = rzalloc_size(mem_ctx, sizeof(nir_xfb_info)); + + xfb->varyings = rzalloc_size(mem_ctx, sizeof(nir_xfb_varying_info) * varying_count); + xfb->outputs = rzalloc_size(mem_ctx, sizeof(nir_xfb_output_info) * output_count); + + return xfb; +} + static void add_var_xfb_outputs(nir_xfb_info *xfb, nir_variable *var, @@ -46,11 +57,11 @@ add_var_xfb_outputs(nir_xfb_info *xfb, } 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->buffers[var->data.xfb_buffer].stride == 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->buffers[var->data.xfb_buffer].stride = var->data.xfb_stride; xfb->buffer_to_stream[var->data.xfb_buffer] = var->data.stream; } @@ -72,6 +83,12 @@ add_var_xfb_outputs(nir_xfb_info *xfb, uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; unsigned location_frac = var->data.location_frac; + nir_xfb_varying_info *varying = >varyings[xfb->varying_count++]; + varying->type = type; + varying->buffer = var->data.xfb_buffer; + varying->offset = *offset; + xfb->buffers[var->data.xfb_buffer].varying_count++; + assert(attrib_slots <= 2); for (unsigned s = 0; s < attrib_slots; s++) { nir_xfb_output_info *output = >outputs[xfb->output_count++]; @@ -132,7 +149,11 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) if (num_outputs == 0) return NULL; - nir_xfb_info *xfb = rzalloc_size(mem_ctx, nir_xfb_info_size(num_outputs)); + /* It is complex to know how many varyings do we have beforehand. We use +* num_outputs as an approximation, as num_outputs should be bigger that +* num_varyings. +*/ + nir_xfb_info *xfb = nir_gather_xfb_info_create(mem_ctx, num_outputs, num_outputs); /* Walk the list of outputs and add them to the array */ nir_foreach_variable(var, >outputs) { diff --git a/src/compiler/nir/nir_xfb_info.h b/src/compiler/nir/nir_xfb_info.h index fef52ba96d8..71f4e87018c 100644 --- a/src/compiler/nir/nir_xfb_info.h +++ b/src/compiler/nir/nir_xfb_info.h @@ -29,6 +29,11 @@ #define NIR_MAX_XFB_BUFFERS 4 #define NIR_MAX_XFB_STREAMS 4 +typedef struct { + uint16_t stride; + uint16_t varying_count; +} nir_xfb_buffer_info; + typedef struct { uint8_t buffer; uint16_t offset; @@ -37,23 +42,26 @@ typedef struct { uint8_t component_offset; } nir_xfb_output_info; +typedef struct { + const struct glsl_type *type; + uint8_t buffer; + uint16_t offset; +} nir_xfb_varying_info; + typedef struct { uint8_t buffers_written; uint8_t streams_written; - uint16_t strides[NIR_MAX_XFB_BUFFERS]; + nir_xfb_buffer_info buffers[NIR_MAX_XFB_BUFFERS]; uint8_t buffer_to_stream[NIR_MAX_XFB_STREAMS]; + uint16_t varying_count; + nir_xfb_varying_info *varyings; + uint16_t output_count; - nir_xfb_output_info outputs[0]; + nir_xfb_output_info *outputs; } 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); --
[Mesa-dev] [PATCH 0/7] RFC: adapt nir_gather_xfb_info to be used by ARB_gl_spirv
ARB_gl_spirv has its own NIR-based xfb gathering info. Since nir_gather_xfb_info landed on master, I have been trying to get to use it, in order to reuse code. Although I consider this series a WIP, I prefer to share what I have right now, just in case the idea of adapt nir_gather_xfb_info is discarded. Having said so, this series also includes a fix ("nir: fix output offset compute for dvec3/4"). That patch can be used even if we discard the rest of the series. The main difference between the custom ARB_gl_spirv code and the NIR general pass are varyings, as the new pass is focused on outputs. As far as I understand, varyings is just needed for OpenGL. So although I was able to modify the new pass to be reused, and all tests we have are passing, I'm not really happy with the current patches. The original idea is that it would be ok to reuse, as far as we didn't need to add too much info, or needed to change too much that pass. So I focused on modifying the code as less as possible. But that lead to some debatable decisions. It is more detailed on the patches with RFC. BR Alejandro Piñeiro (7): spirv/nir: update Xfb decoration comment nir: don't assert when xfb_buffer/stride is present but not xfb_offset nir: fix output offset compute for dvec3/4 nir: add component_offset at nir_xfb_info RFC: nir: adding varyings on nir_xfb_info and gather_info RFC: nir/xfb_info: arrays of basic types adds just one varying nir/linker: use nir_gather_xfb_info src/compiler/glsl/gl_nir_link_xfb.c| 252 ++--- src/compiler/nir/nir_gather_xfb_info.c | 101 +++-- src/compiler/nir/nir_xfb_info.h| 25 ++-- src/compiler/spirv/spirv_to_nir.c | 2 +- 4 files changed, 180 insertions(+), 200 deletions(-) -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/7] nir: don't assert when xfb_buffer/stride is present but not xfb_offset
In order to allow nir_gather_xfb_info to be used on OpenGL, specifically ARB_gl_spirv. So, from OpenGL 4.6 spec, section 11.1.2.1, "Output Variables": "outputs specifying both an *XfbBuffer* and an *Offset* are captured, while outputs not specifying both of these are not captured. Values are captured each time the shader writes to such a decorated object." This implies that are captured if both are present, and not if one of those are lacking. Technically, it doesn't explicitly point that having just one or the other is a mistake. In some cases, glslang is adding some extra XfbBuffer without XfbOffset around, and mentioning that technically that is not a bug (see issue#1526) And for the case of Vulkan, as the same glslang issue mentions, it is not clear if that should be a mistake or not. But even if it is a mistake, it is not really needed to be checked on the driver, and we can let the validation layers to check that. --- src/compiler/nir/nir_gather_xfb_info.c | 23 --- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index e282bba0081..f5d831c6567 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -109,9 +109,16 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) nir_foreach_variable(var, >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); + + /* OpenGL points that both are needed to capture the output, but + * doesn't direcly imply that it is a mistake having one but not the + * other. + */ + if (!var->data.explicit_xfb_buffer || + !var->data.explicit_offset) { +continue; + } + num_outputs += glsl_count_attribute_slots(var->type, false); } } @@ -124,6 +131,16 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) nir_foreach_variable(var, >outputs) { if (var->data.explicit_xfb_buffer || var->data.explicit_xfb_stride) { + + /* OpenGL points that both are needed to capture the output, but + * doesn't direcly imply that it is a mistake having one but not the + * other. + */ + if (!var->data.explicit_xfb_buffer || + !var->data.explicit_offset) { +continue; + } + unsigned location = var->data.location; unsigned offset = var->data.offset; add_var_xfb_outputs(xfb, var, , , var->type); -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying
On OpenGL, a array of a simple type adds just one varying. So gl_transform_feedback_varying_info struct defined at mtypes.h includes the parameters Type (base_type) and Size (number of elements). This commit checks this when the recursive add_var_xfb_outputs call handles arrays, to ensure that just one is addded. RFC: Until this point, all changes were reasonable, but this change is (imho) ugly. My idea was introducing as less as possible changes on the code, specially on its logic/flow. But this commit is almost a hack. The ideal solution would be to change the focus of the recursive function, focusing on varyings, and at each varying, recursively add outputs. But that seems like an overkill for a pass that was originally intended for consumers only caring about the outputs. So perhaps ARB_gl_spirv should keep their own gathering pass, with vayings and outputs, and let this one untouched for those that only care on outputs. --- src/compiler/nir/nir_gather_xfb_info.c | 52 -- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/src/compiler/nir/nir_gather_xfb_info.c b/src/compiler/nir/nir_gather_xfb_info.c index 948b802a815..cb0e2724cab 100644 --- a/src/compiler/nir/nir_gather_xfb_info.c +++ b/src/compiler/nir/nir_gather_xfb_info.c @@ -36,23 +36,59 @@ nir_gather_xfb_info_create(void *mem_ctx, uint16_t output_count, uint16_t varyin return xfb; } +static bool +glsl_type_is_leaf(const struct glsl_type *type) +{ + if (glsl_type_is_struct(type) || + (glsl_type_is_array(type) && +(glsl_type_is_array(glsl_get_array_element(type)) || + glsl_type_is_struct(glsl_get_array_element(type) { + return false; + } else { + return true; + } +} + +static void +add_var_xfb_varying(nir_xfb_info *xfb, +nir_variable *var, +unsigned offset, +const struct glsl_type *type) +{ + nir_xfb_varying_info *varying = >varyings[xfb->varying_count++]; + + varying->type = type; + varying->buffer = var->data.xfb_buffer; + varying->offset = offset; + xfb->buffers[var->data.xfb_buffer].varying_count++; +} + static void add_var_xfb_outputs(nir_xfb_info *xfb, nir_variable *var, unsigned *location, unsigned *offset, -const struct glsl_type *type) +const struct glsl_type *type, +bool varying_added) { if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) { unsigned length = glsl_get_length(type); + bool local_varying_added = varying_added; + const struct glsl_type *child_type = glsl_get_array_element(type); + if (glsl_type_is_leaf(child_type)) { + + add_var_xfb_varying(xfb, var, *offset, type); + local_varying_added = true; + } + for (unsigned i = 0; i < length; i++) - add_var_xfb_outputs(xfb, var, location, offset, child_type); + add_var_xfb_outputs(xfb, var, location, offset, child_type, local_varying_added); } 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); + add_var_xfb_outputs(xfb, var, location, offset, child_type, varying_added); } } else { assert(var->data.xfb_buffer < NIR_MAX_XFB_BUFFERS); @@ -83,11 +119,9 @@ add_var_xfb_outputs(nir_xfb_info *xfb, uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac; unsigned location_frac = var->data.location_frac; - nir_xfb_varying_info *varying = >varyings[xfb->varying_count++]; - varying->type = type; - varying->buffer = var->data.xfb_buffer; - varying->offset = *offset; - xfb->buffers[var->data.xfb_buffer].varying_count++; + if (!varying_added) { + add_var_xfb_varying(xfb, var, *offset, type); + } assert(attrib_slots <= 2); for (unsigned s = 0; s < attrib_slots; s++) { @@ -171,7 +205,7 @@ nir_gather_xfb_info(const nir_shader *shader, void *mem_ctx) unsigned location = var->data.location; unsigned offset = var->data.offset; - add_var_xfb_outputs(xfb, var, , , var->type); + add_var_xfb_outputs(xfb, var, , , var->type, false); } } assert(xfb->output_count == num_outputs); -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] spirv/nir: don't set interface_type if it is not a struct
vnt_variables uses interface_type on several use cases, but on nir variable it is more limited. From nir.h: /** * For variables that are in an interface block or are an instance of an * interface block, this is the \c GLSL_TYPE_INTERFACE type for that block. * * \sa ir_variable::location */ But interface blocks expects the type to be an struct, so those cases should not be filled. For example, glsl checks if a variable is in an uniform block if it is an uniform and has an interface type. One example of why this is needed: gl_PatchVerticesIn is lowered to an uniform. Without this change, it would include a interface_type. Then, we would try to initialize the uniform block, and find that it doesn't have any component. v2: rearrange/clean code to only set interface_type for structs, instead of a default assignment, and a NULL reassignement for non-structs (Timothy) Reviewed-by: Timothy Arceri --- src/compiler/spirv/vtn_variables.c | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 0606ae0e243..541ba73e643 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1789,6 +1789,12 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, var_is_patch_cb, >patch); } + var->var = rzalloc(b->shader, nir_variable); + var->var->name = ralloc_strdup(var->var, val->name); + var->var->type = var->type->type; + var->var->data.mode = nir_mode; + var->var->data.patch = var->patch; + /* For inputs and outputs, we immediately split structures. This * is for a couple of reasons. For one, builtins may all come in * a struct and we really want those split out into separate @@ -1808,14 +1814,9 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, interface_type = var->type->array_element; } - var->var = rzalloc(b->shader, nir_variable); - var->var->name = ralloc_strdup(var->var, val->name); - var->var->type = var->type->type; - var->var->interface_type = interface_type->type; - var->var->data.mode = nir_mode; - var->var->data.patch = var->patch; - if (glsl_type_is_struct(interface_type->type)) { + var->var->interface_type = interface_type->type; + /* It's a struct. Set it up as per-member. */ var->var->num_members = glsl_get_length(interface_type->type); var->var->members = rzalloc_array(var->var, struct nir_variable_data, -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] glspirv: no need to force entrypoint name to "main"
Since commit "intel/compiler: Stop assuming the entrypoint is called "main"" there is no need to force the entrypoint name to be "main". --- src/mesa/main/glspirv.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c index 98b7ea77348..04e46ba571e 100644 --- a/src/mesa/main/glspirv.c +++ b/src/mesa/main/glspirv.c @@ -253,7 +253,6 @@ _mesa_spirv_to_nir(struct gl_context *ctx, exec_node_remove(>node); } assert(exec_list_length(>functions) == 1); - entry_point->name = ralloc_strdup(entry_point, "main"); /* Split member structs. We do this before lower_io_to_temporaries so that * it doesn't lower system values to temporaries by accident. -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 23/28] nir/linker: add program ubo/ssbo at the resource list
--- src/compiler/glsl/gl_nir_linker.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/src/compiler/glsl/gl_nir_linker.c b/src/compiler/glsl/gl_nir_linker.c index 547549bc4e0..138a12e532d 100644 --- a/src/compiler/glsl/gl_nir_linker.c +++ b/src/compiler/glsl/gl_nir_linker.c @@ -67,5 +67,19 @@ nir_build_program_resource_list(struct gl_context *ctx, } + /* Add program uniform blocks. */ + for (unsigned i = 0; i < prog->data->NumUniformBlocks; i++) { + if (!link_util_add_program_resource(prog, resource_set, GL_UNIFORM_BLOCK, + >data->UniformBlocks[i], 0)) + return; + } + + /* Add program shader storage blocks. */ + for (unsigned i = 0; i < prog->data->NumShaderStorageBlocks; i++) { + if (!link_util_add_program_resource(prog, resource_set, GL_SHADER_STORAGE_BLOCK, + >data->ShaderStorageBlocks[i], 0)) + return; + } + _mesa_set_destroy(resource_set, NULL); } -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 25/28] i965: call to gl_nir_link_uniform_blocks
When using a SPIR-V shader. Note that needs to be done before linking uniforms, so when creating the uniform storage entries, block_index could be filled properly (among other things). --- src/mesa/drivers/dri/i965/brw_link.cpp | 4 1 file changed, 4 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp index 03b32d1fe7a..d0179cc89a1 100644 --- a/src/mesa/drivers/dri/i965/brw_link.cpp +++ b/src/mesa/drivers/dri/i965/brw_link.cpp @@ -263,6 +263,10 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) /* SPIR-V programs use a NIR linker */ if (shProg->data->spirv) { + if (!gl_nir_link_uniform_blocks(ctx, shProg)) { + return GL_FALSE; + } + if (!gl_nir_link_uniforms(ctx, shProg)) return GL_FALSE; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 27/28] mesa: add NULL name check for several length queries
Since ARB_gl_spirv it is possible to miss a lot of name reflection information, so it is needed to add NULL name checks for several queries, and return a specific value on those cases. This commit add them for ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH, ACTIVE_ATTRIBUTE_MAX_LENGTH and ACTIVE_UNIFORM_MAX_LENGTH. From ARB_gl_spirv spec: "If pname is ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH, the length of the longest active uniform block name, including the null terminator, is returned. If no active uniform blocks exist, zero is returned. If no name reflection information is available, one is returned. If pname is ACTIVE_ATTRIBUTE_MAX_LENGTH, the length of the longest active attribute name, including a null terminator, is returned. If no active attributes exist, zero is returned. If no name reflection information is available, one is returned. If pname is ACTIVE_UNIFORM_MAX_LENGTH, the length of the longest active uniform name, including a null terminator, is returned. If no active uniforms exist, zero is returned. If no name reflection information is available, one is returned." --- src/mesa/main/shader_query.cpp | 12 ++-- src/mesa/main/shaderapi.c | 26 ++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index b775b4231c2..0a85e183a0c 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -244,9 +244,17 @@ _mesa_longest_attribute_name_length(struct gl_shader_program *shProg) if (res->Type == GL_PROGRAM_INPUT && res->StageReferences & (1 << MESA_SHADER_VERTEX)) { - const size_t length = strlen(RESOURCE_VAR(res)->name); + /* From ARB_gl_spirv spec: + * "If pname is ACTIVE_ATTRIBUTE_MAX_LENGTH, the length of the + *longest active attribute name, including a null terminator, is + *returned. If no active attributes exist, zero is returned. If + *no name reflection information is available, one is returned." + */ + const size_t length = RESOURCE_VAR(res)->name != NULL ? + strlen(RESOURCE_VAR(res)->name) : 1; + if (length >= longest) - longest = length + 1; + longest = RESOURCE_VAR(res)->name != NULL ? length + 1 : length; } } diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c index 2ea8d965aba..3e532c1b41e 100644 --- a/src/mesa/main/shaderapi.c +++ b/src/mesa/main/shaderapi.c @@ -728,11 +728,22 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname, if (shProg->data->UniformStorage[i].is_shader_storage) continue; + /* From ARB_gl_spirv spec: + * "If pname is ACTIVE_UNIFORM_MAX_LENGTH, the length of the + *longest active uniform name, including a null terminator, is + *returned. If no active uniforms exist, zero is returned. If no + *name reflection information is available, one is returned." + * + * We are setting 0 here, as below it will add 1 for the NUL character. + */ + const GLint base_len = shProg->data->UniformStorage[i].name != NULL ? +strlen(shProg->data->UniformStorage[i].name) : 0; + /* Add one for the terminating NUL character for a non-array, and * 4 for the "[0]" and the NUL for an array. */ - const GLint len = strlen(shProg->data->UniformStorage[i].name) + 1 + - ((shProg->data->UniformStorage[i].array_elements != 0) ? 3 : 0); + const GLint len = base_len + 1 + +((shProg->data->UniformStorage[i].array_elements != 0) ? 3 : 0); if (len > max_len) max_len = len; @@ -810,9 +821,16 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname, break; for (i = 0; i < shProg->data->NumUniformBlocks; i++) { -/* Add one for the terminating NUL character. +/* Add one for the terminating NUL character. Name can be NULL, in + * that case, from ARB_gl_spirv: + * "If pname is ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH, the length of + *the longest active uniform block name, including the null + *terminator, is returned. If no active uniform blocks exist, + *zero is returned. If no name reflection information is + *available, one is returned." */ - const GLint len = strlen(shProg->data->UniformBlocks[i].Name) + 1; + const GLint len = shProg->data->UniformBlocks[i].Name ? +strlen(shProg->data->UniformBlocks[i].Name) + 1 : 1; if (len > max_len) max_len = len; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 26/28] mesa: add NULL name check for NUM_ACTIVE_VARIABLES query
This can happens if we are running an SPIR-V shader (ARB_gl_spirv). --- src/mesa/main/shader_query.cpp | 30 -- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index 11ecd71c575..b775b4231c2 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -1013,11 +1013,16 @@ get_buffer_property(struct gl_shader_program *shProg, *val = 0; for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms; i++) { const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName; -struct gl_program_resource *uni = - _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname, -NULL); -if (!uni) - continue; +/* IndexName can be NULL if we are using a SPIR-V shader + * (ARB_gl_spirv). + */ +if (iname != NULL) { + struct gl_program_resource *uni = + _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname, + NULL); + if (!uni) + continue; +} (*val)++; } return 1; @@ -1049,11 +1054,16 @@ get_buffer_property(struct gl_shader_program *shProg, *val = 0; for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms; i++) { const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName; -struct gl_program_resource *uni = - _mesa_program_resource_find_name(shProg, GL_BUFFER_VARIABLE, -iname, NULL); -if (!uni) - continue; +/* IndexName can be NULL if we are using a SPIR-V shader + * (ARB_gl_spirv). + */ +if (iname != NULL) { + struct gl_program_resource *uni = + _mesa_program_resource_find_name(shProg, GL_BUFFER_VARIABLE, + iname, NULL); + if (!uni) + continue; +} (*val)++; } return 1; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 28/28] nir/linker: Add inputs/outputs to the program resource list
From: Antia Puentes --- src/compiler/glsl/gl_nir_linker.c | 79 +++ 1 file changed, 79 insertions(+) diff --git a/src/compiler/glsl/gl_nir_linker.c b/src/compiler/glsl/gl_nir_linker.c index 138a12e532d..acec0fe1f03 100644 --- a/src/compiler/glsl/gl_nir_linker.c +++ b/src/compiler/glsl/gl_nir_linker.c @@ -33,6 +33,58 @@ * Also note that this is tailored for ARB_gl_spirv needs and particularities */ +static bool +add_interface_variables(const struct gl_context *cts, +struct gl_shader_program *prog, +struct set *resource_set, +unsigned stage, GLenum programInterface) +{ + const struct exec_list *var_list = NULL; + + struct gl_linked_shader *sh = prog->_LinkedShaders[stage]; + if (!sh) + return true; + + nir_shader *nir = sh->Program->nir; + assert(nir); + + switch (programInterface) { + case GL_PROGRAM_INPUT: + var_list = >inputs; + break; + case GL_PROGRAM_OUTPUT: + var_list = >outputs; + break; + default: + assert("!Should not get here"); + break; + } + + nir_foreach_variable(var, var_list) { + if (var->data.how_declared == nir_var_hidden) + continue; + + struct gl_shader_variable *sh_var = + rzalloc(prog, struct gl_shader_variable); + + /* In the ARB_gl_spirv spec, names are considered optional debug info, so + * the linker needs to work without them. Returning them is optional. + * For simplicity, we ignore names. + */ + sh_var->name = NULL; + sh_var->type = var->type; + sh_var->location = var->data.location; + + if (!link_util_add_program_resource(prog, resource_set, + programInterface, + sh_var, 1 << stage)) { + return false; + } + } + + return true; +} + void nir_build_program_resource_list(struct gl_context *ctx, struct gl_shader_program *prog) @@ -44,10 +96,37 @@ nir_build_program_resource_list(struct gl_context *ctx, prog->data->NumProgramResourceList = 0; } + int input_stage = MESA_SHADER_STAGES, output_stage = 0; + + /* Determine first input and final output stage. These are used to +* detect which variables should be enumerated in the resource list +* for GL_PROGRAM_INPUT and GL_PROGRAM_OUTPUT. +*/ + for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) { + if (!prog->_LinkedShaders[i]) + continue; + if (input_stage == MESA_SHADER_STAGES) + input_stage = i; + output_stage = i; + } + + /* Empty shader, no resources. */ + if (input_stage == MESA_SHADER_STAGES && output_stage == 0) + return; + struct set *resource_set = _mesa_set_create(NULL, _mesa_hash_pointer, _mesa_key_pointer_equal); + /* Add inputs and outputs to the resource list. */ + if (!add_interface_variables(ctx, prog, resource_set, input_stage, +GL_PROGRAM_INPUT)) + return; + + if (!add_interface_variables(ctx, prog, resource_set, output_stage, +GL_PROGRAM_OUTPUT)) + return; + /* Add uniforms * * Here, it is expected that nir_link_uniforms() has already been -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 24/28] i965: use GLboolean for all brw_link_shader returns
The function had a mix of true/GL_TRUE and false/GL_FALSE returns. Using GL_TRUE/GL_FALSE as the function returns a GLboolean. --- src/mesa/drivers/dri/i965/brw_link.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp index 37b775637b4..03b32d1fe7a 100644 --- a/src/mesa/drivers/dri/i965/brw_link.cpp +++ b/src/mesa/drivers/dri/i965/brw_link.cpp @@ -264,7 +264,7 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) /* SPIR-V programs use a NIR linker */ if (shProg->data->spirv) { if (!gl_nir_link_uniforms(ctx, shProg)) - return false; + return GL_FALSE; gl_nir_link_assign_atomic_counter_resources(ctx, shProg); gl_nir_link_assign_xfb_resources(ctx, shProg); @@ -375,7 +375,7 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) } if (brw->precompile && !brw_shader_precompile(ctx, shProg)) - return false; + return GL_FALSE; /* SPIR-V programs build its resource list from linked NIR shaders. */ if (!shProg->data->spirv) @@ -393,5 +393,5 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) shader->ir = NULL; } - return true; + return GL_TRUE; } -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 21/28] nir/linker: update already processed uniforms search for UBOs/SSBOs
Until now, we were using the uniform explicit location to check if the current nir variable already was processed, and entries on the uniform storage added. But for UBOs/SSBOs, entries are added but we lack a explicit location. For those we need to rely on the UBO/SSBO binding (to the nir variable binding, and the uniform storage block_index). In that case several uniforms would need to be updated at once. --- src/compiler/glsl/gl_nir_link_uniforms.c | 78 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index 448f8277c16..d266091ba80 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -130,20 +130,79 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx, } } +static void +update_uniform_storage(struct gl_uniform_storage *uniform, + unsigned stage) +{ + uniform->active_shader_mask |= 1 << stage; +} + +/** + * Finds, return, and update the stage infor for any uniform at the + * UniformStorage any uniform defined by @var. In general this is done using + * the explicit location, except: + * + * * UBOs/SSBOs: as they lack explicit location, binding is used to locate + * them. That means that more that one entry at the uniform storage can be + * found. In that case all of them are updated, and the first entry is + * returned, in order to update the location of nir variable. + * + * * Expecial uniforms: like atomic counters. They lack a explicit location, + * so they are skipped, handled in any case, and assign a location later. + * + */ static struct gl_uniform_storage * -find_previous_uniform_storage(struct gl_shader_program *prog, - int location) +find_and_update_previous_uniform_storage(struct gl_shader_program *prog, + nir_variable *var, + unsigned stage) { - /* This would only work for uniform with explicit location, as all the -* uniforms without location (ie: atomic counters) would have a initial -* location equal to -1. We early return in that case. + if (nir_variable_is_in_block(var)) { + struct gl_uniform_storage *uniform = NULL; + + unsigned num_blks = nir_variable_is_in_ubo(var) ? + prog->data->NumUniformBlocks : + prog->data->NumShaderStorageBlocks; + + struct gl_uniform_block *blks = nir_variable_is_in_ubo(var) ? + prog->data->UniformBlocks : prog->data->ShaderStorageBlocks; + + for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) { + /* UniformStorage contains both variables from ubos and ssbos */ + if ( prog->data->UniformStorage[i].is_shader_storage != + nir_variable_is_in_ssbo(var)) +continue; + + int block_index = prog->data->UniformStorage[i].block_index; + if (block_index != -1) { +assert(block_index < num_blks); + +if (var->data.binding == blks[block_index].Binding) { + if (!uniform) + uniform = >data->UniformStorage[i]; + update_uniform_storage(>data->UniformStorage[i], + stage); +} + } + } + + return uniform; + } + + /* Beyond blocks, there are still some corner cases of uniforms without +* location (ie: atomic counters) that would have a initial location equal +* to -1. We just return on that case. Those uniforms will be handled +* later. */ - if (location == -1) + if (var->data.location == -1) return NULL; - for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) - if (prog->data->UniformStorage[i].remap_location == location) + for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) { + if (prog->data->UniformStorage[i].remap_location == var->data.location) { + update_uniform_storage(>data->UniformStorage[i], stage); + return >data->UniformStorage[i]; + } + } return NULL; } @@ -504,9 +563,8 @@ gl_nir_link_uniforms(struct gl_context *ctx, * other stage. If so, validate they are compatible and update * the active stage mask. */ - uniform = find_previous_uniform_storage(prog, var->data.location); + uniform = find_and_update_previous_uniform_storage(prog, var, shader_type); if (uniform) { -uniform->active_shader_mask |= 1 << shader_type; var->data.location = uniform - prog->data->UniformStorage; continue; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 20/28] nir/linker: fill up uniform_storage with explicit data
Specifically, offset, array_stride, matrix_stride and row_major. On GLSL, most of that info is computed, but on ARB_gl_spirv they are explicit, and for Mesa, included on the glsl_type. From ARB_gl_spirv spec: "Mapping of layouts std140/std430 -> explicit *Offset*, *ArrayStride*, and *MatrixStride* Decoration on struct members"" "7.6.2.spv SPIR-V Uniform Offsets and Strides The SPIR-V decorations *GLSLShared* or *GLSLPacked* must not be used. A variable in the *Uniform* Storage Class decorated as a *Block* must be explicitly laid out using the *Offset*, *ArrayStride*, and *MatrixStride* decorations" For offset, matrix_stride and row_major we needed to include the parent and index_in_parent while processing the type, as matrix_stride/row_major are maintained as fields of the parent type, not on the type itself. --- src/compiler/glsl/gl_nir_link_uniforms.c | 31 ++- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index ac445c8560a..448f8277c16 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -282,6 +282,8 @@ nir_link_uniform(struct gl_context *ctx, struct gl_program *stage_program, gl_shader_stage stage, const struct glsl_type *type, + const struct glsl_type *parent_type, + unsigned index_in_parent, int location, struct nir_link_uniforms_state *state) { @@ -309,7 +311,7 @@ nir_link_uniform(struct gl_context *ctx, field_type = glsl_get_array_element(type); int entries = nir_link_uniform(ctx, prog, stage_program, stage, -field_type, location, +field_type, type, i, location, state); if (entries == -1) return -1; @@ -352,9 +354,11 @@ nir_link_uniform(struct gl_context *ctx, if (glsl_type_is_array(type)) { uniform->type = type_no_array; uniform->array_elements = glsl_get_length(type); + uniform->array_stride = glsl_get_explicit_array_stride(type); } else { uniform->type = type; uniform->array_elements = 0; + uniform->array_stride = 0; } uniform->active_shader_mask |= 1 << stage; @@ -371,15 +375,31 @@ nir_link_uniform(struct gl_context *ctx, uniform->is_shader_storage = nir_variable_is_in_ssbo(state->current_var); + if (nir_variable_is_in_block(state->current_var) && + glsl_type_is_matrix(type)) { + assert(parent_type); + + uniform->matrix_stride = +glsl_get_struct_field_explicit_matrix_stride(parent_type, index_in_parent); + + uniform->row_major = +glsl_get_struct_field_matrix_layout(parent_type, index_in_parent) == +GLSL_MATRIX_LAYOUT_ROW_MAJOR; + } else { + uniform->matrix_stride = 0; + uniform->row_major = false; + } + + if (parent_type) + uniform->offset = glsl_get_struct_field_offset(parent_type, index_in_parent); + else + uniform->offset = 0; + /* @FIXME: the initialization of the following will be done as we * implement support for their specific features, like SSBO, atomics, * etc. */ uniform->block_index = -1; - uniform->offset = -1; - uniform->matrix_stride = -1; - uniform->array_stride = -1; - uniform->row_major = false; uniform->builtin = false; uniform->atomic_buffer_index = -1; uniform->top_level_array_size = 0; @@ -543,6 +563,7 @@ gl_nir_link_uniforms(struct gl_context *ctx, state.current_type = type_tree; int res = nir_link_uniform(ctx, prog, sh->Program, shader_type, type, +NULL, 0, location, ); free_type_tree(type_tree); -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 22/28] nir/linker: Set the uniform's block_index
From: Antia Puentes Binding comparison is used to determine the block the uniform is part of. To do the binding comparison we need the information in UniformBlocks[] and ShaderStorageBlocks[] to be available, so we have to call gl_nir_link_uniform_blocks() before linking the uniforms. --- src/compiler/glsl/gl_nir_link_uniforms.c | 22 +- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index d266091ba80..77def1a623f 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -454,11 +454,31 @@ nir_link_uniform(struct gl_context *ctx, else uniform->offset = 0; + int buffer_block_index = -1; + /* If the uniform is inside a uniform block determine its block index by + * comparing the bindings, we can not use names. + */ + if (nir_variable_is_in_block(state->current_var)) { + struct gl_uniform_block *blocks = nir_variable_is_in_ssbo(state->current_var) ? +prog->data->ShaderStorageBlocks : prog->data->UniformBlocks; + + int num_blocks = nir_variable_is_in_ssbo(state->current_var) ? +prog->data->NumShaderStorageBlocks : prog->data->NumUniformBlocks; + + for (unsigned i = 0; i < num_blocks; i++) { +if (state->current_var->data.binding == blocks[i].Binding) { + buffer_block_index = i; +} + } + assert(buffer_block_index >= 0); + } + + uniform->block_index = buffer_block_index; + /* @FIXME: the initialization of the following will be done as we * implement support for their specific features, like SSBO, atomics, * etc. */ - uniform->block_index = -1; uniform->builtin = false; uniform->atomic_buffer_index = -1; uniform->top_level_array_size = 0; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 19/28] nir/linker: use only the array element type for array of ssbo/ubo
For this interfaces, the inner members are added only once as uniforms or resources, in opposite to other cases, like a uniform array of structs. For those guessing why a issue (16) from ARB_program_interface_query was used, instead of a quote of the core spec: The core spec is not really clear about how members of arrays of blocks should be enumerated. On GLSL this was also problematic, specially when we were trying to pass the 4.5 CTS tests. See commit "glsl: Fix program interface queries relating to interface blocks" (4c4d9e4f032d5753034361ee70aa88d16d3a04b4), as a reference. That one also needed to rely on issue (16) to justify the change, pointing that the core spec needs to be clarified. --- src/compiler/glsl/gl_nir_link_uniforms.c | 44 ++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index 00995fb3f76..ac445c8560a 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -498,11 +498,51 @@ gl_nir_link_uniforms(struct gl_context *ctx, state.current_var = var; + /* + * From ARB_program_interface spec, issue (16): + * + * "RESOLVED: We will follow the default rule for enumerating block + * members in the OpenGL API, which is: + * + * * If a variable is a member of an interface block without an + *instance name, it is enumerated using just the variable name. + * + * * If a variable is a member of an interface block with an + *instance name, it is enumerated as "BlockName.Member", where + *"BlockName" is the name of the interface block (not the + *instance name) and "Member" is the name of the variable. + * + * For example, in the following code: + * + * uniform Block1 { + * int member1; + * }; + * uniform Block2 { + * int member2; + * } instance2; + * uniform Block3 { + * int member3; + * } instance3[2]; // uses two separate buffer bindings + * + * the three uniforms (if active) are enumerated as "member1", + * "Block2.member2", and "Block3.member3"." + * + * Note that in the last example, with an array of ubo, only one + * uniform is generated. For that reason, while unrolling the + * uniforms of a ubo, or the variables of a ssbo, we need to treat + * arrays of instance as a single block. + */ + const struct glsl_type *type = var->type; + if (nir_variable_is_in_block(var) && + glsl_type_is_array(type)) { +type = glsl_without_array(type); + } + struct type_tree_entry *type_tree = -build_type_tree_for_type(var->type); +build_type_tree_for_type(type); state.current_type = type_tree; - int res = nir_link_uniform(ctx, prog, sh->Program, shader_type, var->type, + int res = nir_link_uniform(ctx, prog, sh->Program, shader_type, type, location, ); free_type_tree(type_tree); -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 10/28] glsl_types/nir: add matrix_stride plus nir wrapper helpers
From ARB_gl_spirv spec: "7.6.2.spv SPIR-V Uniform Offsets and Strides The SPIR-V decorations *GLSLShared* or *GLSLPacked* must not be used. A variable in the *Uniform* Storage Class decorated as a *Block* must be explicitly laid out using the *Offset*, *ArrayStride*, and *MatrixStride* decorations. If the variable is decorated as a *BufferBlock*, its offsets and strides must not contradict std430 alignment and minimum offset requirements. Otherwise, its offsets and strides must not contradict std140 alignment and minimum offset requirements. From that paragraph, the first conclusion is that we can rely on the content of the SPIR-V in order to compute the buffer sizes, as they are mandatory. That would make the buffer size computation easier. The second conclusion, from the last sentence, is that *we need* to do that. As if just needs to not contradict alignments and minimum offsets, providing a matrix stride of 16 when 8 is enough would be valid. This explicit matrix_stride is assumed to only be used on ARB_gl_spirv. On GLSL there is no way to set it, and it is internally handled and computed. --- src/compiler/glsl_types.cpp | 3 +++ src/compiler/glsl_types.h | 10 -- src/compiler/nir_types.cpp | 6 ++ src/compiler/nir_types.h| 2 ++ 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 70bce6ace8e..104a5104aaa 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -961,6 +961,9 @@ glsl_type::record_compare(const glsl_type *b, bool match_locations) const if (this->fields.structure[i].xfb_stride != b->fields.structure[i].xfb_stride) return false; + if (this->fields.structure[i].explicit_matrix_stride + != b->fields.structure[i].explicit_matrix_stride) + return false; } return true; diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h index d32b580acc1..9e8332e6cbf 100644 --- a/src/compiler/glsl_types.h +++ b/src/compiler/glsl_types.h @@ -1007,6 +1007,12 @@ struct glsl_struct_field { */ unsigned matrix_layout:2; + /** +* Explicit matrix stride. For ARB_gl_spirv, it is mandatory to set it +* explicitly. -1 otherwise. +*/ + int explicit_matrix_stride; + /** * For interface blocks, 1 if this variable is a per-patch input or output * (as in ir_variable::patch). 0 otherwise. @@ -1045,7 +1051,7 @@ struct glsl_struct_field { glsl_struct_field(const struct glsl_type *_type, const char *_name) : type(_type), name(_name), location(-1), offset(0), xfb_buffer(0), xfb_stride(0), interpolation(0), centroid(0), -sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0), +sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), explicit_matrix_stride(-1), patch(0), precision(GLSL_PRECISION_NONE), memory_read_only(0), memory_write_only(0), memory_coherent(0), memory_volatile(0), memory_restrict(0), image_format(0), explicit_xfb_buffer(0), @@ -1057,7 +1063,7 @@ struct glsl_struct_field { glsl_struct_field() : type(NULL), name(NULL), location(0), offset(0), xfb_buffer(0), xfb_stride(0), interpolation(0), centroid(0), -sample(0), matrix_layout(0), patch(0), +sample(0), matrix_layout(0), explicit_matrix_stride(-1), patch(0), precision(0), memory_read_only(0), memory_write_only(0), memory_coherent(0), memory_volatile(0), memory_restrict(0), image_format(0), explicit_xfb_buffer(0), diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp index 2a1ae42a9bb..b2a5da1dc6c 100644 --- a/src/compiler/nir_types.cpp +++ b/src/compiler/nir_types.cpp @@ -86,6 +86,12 @@ glsl_get_struct_field_matrix_layout(const struct glsl_type *type, return type->fields.structure[index].matrix_layout; } +const int +glsl_get_struct_field_explicit_matrix_stride(const struct glsl_type *type, + unsigned index) +{ + return type->fields.structure[index].explicit_matrix_stride; +} const glsl_type * glsl_get_function_return_type(const glsl_type *type) diff --git a/src/compiler/nir_types.h b/src/compiler/nir_types.h index 69de44c3423..d3c00ca5e1a 100644 --- a/src/compiler/nir_types.h +++ b/src/compiler/nir_types.h @@ -51,6 +51,8 @@ const int glsl_get_struct_field_offset(const struct glsl_type *type, const unsigned glsl_get_struct_field_matrix_layout(const struct glsl_type *type, unsigned index); +const int glsl_get_struct_field_explicit_matrix_stride(const struct glsl_type *type, + unsigned index); const struct glsl_type *glsl_get_array_element(const struct glsl_type *type); const struct glsl_type *glsl_without_array(const struct glsl_type *type); const struct glsl_type
[Mesa-dev] [PATCH 11/28] spirv/nir: fill glsl_struct_field explicit_matrix_stride
--- src/compiler/spirv/spirv_to_nir.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 52c3c968bb7..1201143d2f4 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -816,6 +816,12 @@ struct_member_matrix_stride_cb(struct vtn_builder *b, vtn_assert(mat_type->array_element->stride > 0); mat_type->stride = dec->literals[0]; } + + /* For the glsl_type we use the stride defined at SPIR-V, as anyone (ie: +* ARB_gl_spirv linker) that wants to use it would be also using the matrix +* layout. +*/ + ctx->fields[member].explicit_matrix_stride = dec->literals[0]; } static void -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 12/28] glsl_types/nir: add explicit_array_stride plus nir wrapper helpers
From ARB_gl_spirv: "Mapping of layouts std140/std430 -> explicit *Offset*, *ArrayStride*, and *MatrixStride* Decoration on struct members" That means that we would not have available any kind of layout info, and we should use explicit array strides. This commit adds explicit_array_stride. The default value is -1 meaning that it is not set (as with offset). That should be the default value for GLSL. In general, the default constructor is ok. We just need to be careful with some array lowerings, as it should try to get the explicit array stride when creating new types. Note that this means that for the ARB_gl_spirv case std430_array_stride, std140_size etc are meaningless (unless you guess the layout, something that you shouldn't). v2: add missing glsl_full_array_type call, found while testing ARB_gl_spirv with borrowed tests (Alejandro) --- src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +- src/compiler/glsl_types.cpp| 28 +- src/compiler/glsl_types.h | 13 +++--- src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 ++- src/compiler/nir/nir_split_per_member_structs.c| 3 ++- src/compiler/nir/nir_split_vars.c | 7 -- src/compiler/nir_types.cpp | 20 +--- src/compiler/nir_types.h | 10 +++- src/compiler/spirv/vtn_variables.c | 3 ++- 9 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c b/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c index 9ff5708f503..9716ac4562a 100644 --- a/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c +++ b/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c @@ -99,7 +99,7 @@ remove_struct_derefs_prep(nir_deref_instr **p, char **name, remove_struct_derefs_prep([1], name, location, type); - *type = glsl_get_array_instance(*type, length); + *type = glsl_get_array_instance(*type, length, glsl_get_explicit_array_stride(cur->type)); break; } diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index 104a5104aaa..ef3058a3911 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -44,7 +44,7 @@ glsl_type::glsl_type(GLenum gl_type, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(vector_elements), matrix_columns(matrix_columns), - length(0) + length(0), explicit_array_stride(-1) { /* Values of these types must fit in the two bits of * glsl_type::sampled_type. @@ -77,7 +77,7 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, base_type(base_type), sampled_type(type), sampler_dimensionality(dim), sampler_shadow(shadow), sampler_array(array), interface_packing(0), - interface_row_major(0), length(0) + interface_row_major(0), length(0), explicit_array_stride(-1) { this->mem_ctx = ralloc_context(NULL); assert(this->mem_ctx != NULL); @@ -97,7 +97,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(num_fields) + length(num_fields), explicit_array_stride(-1) { unsigned int i; @@ -127,7 +127,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, interface_packing((unsigned) packing), interface_row_major((unsigned) row_major), vector_elements(0), matrix_columns(0), - length(num_fields) + length(num_fields), explicit_array_stride(-1) { unsigned int i; @@ -152,7 +152,7 @@ glsl_type::glsl_type(const glsl_type *return_type, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(num_params) + length(num_params), explicit_array_stride(-1) { unsigned int i; @@ -181,7 +181,7 @@ glsl_type::glsl_type(const char *subroutine_name) : sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(1), matrix_columns(1), - length(0) + length(0), explicit_array_stride(-1) { this->mem_ctx = ralloc_context(NULL); assert(this->mem_ctx != NULL); @@ -434,12 +434,12 @@ _mesa_glsl_release_types(void) } -glsl_type::glsl_type(const glsl_type *array, unsigned length) : +glsl_type::glsl_type(const glsl_type *array, unsigned length, int explicit_array_stride) : base_type(GLSL_TYPE_ARRAY), sampled_type(GLSL_TYPE_VOID), sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(length), name(NULL) + length(length),
[Mesa-dev] [PATCH 15/28] nir/linker/i965: Lower vulkan_resource_index during linking
From: Neil Roberts When linking a program using ARB_gl_spirv it now lowers the vulkan_resource_index intrinsic as an extra pass on the nir shader. Unlike Vulkan this can be done without waiting for the extra state from the pipeline layout. It also adds the call to this lowering on the i965 driver, to avoid a new two-liner patch. --- src/compiler/Makefile.sources | 1 + src/compiler/glsl/gl_nir.h | 4 + .../glsl/gl_nir_lower_vulkan_resource_index.c | 120 + src/compiler/glsl/meson.build | 1 + src/mesa/drivers/dri/i965/brw_link.cpp | 2 + 5 files changed, 128 insertions(+) create mode 100644 src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index b65bb9b80b9..3021bede6cf 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -28,6 +28,7 @@ LIBGLSL_FILES = \ glsl/gl_nir_lower_atomics.c \ glsl/gl_nir_lower_samplers.c \ glsl/gl_nir_lower_samplers_as_deref.c \ + glsl/gl_nir_lower_vulkan_resource_index.c \ glsl/gl_nir_link_atomics.c \ glsl/gl_nir_link_uniform_initializers.c \ glsl/gl_nir_link_uniforms.c \ diff --git a/src/compiler/glsl/gl_nir.h b/src/compiler/glsl/gl_nir.h index 59d5f65e659..80f56039952 100644 --- a/src/compiler/glsl/gl_nir.h +++ b/src/compiler/glsl/gl_nir.h @@ -30,6 +30,7 @@ extern "C" { struct nir_shader; struct gl_shader_program; +struct gl_linked_shader; bool gl_nir_lower_atomics(nir_shader *shader, const struct gl_shader_program *shader_program, @@ -40,6 +41,9 @@ bool gl_nir_lower_samplers(nir_shader *shader, bool gl_nir_lower_samplers_as_deref(nir_shader *shader, const struct gl_shader_program *shader_program); +bool gl_nir_lower_vulkan_resource_index(nir_shader *shader, +struct gl_linked_shader *linked_shader); + #ifdef __cplusplus } #endif diff --git a/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c new file mode 100644 index 000..92ee3dd707a --- /dev/null +++ b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c @@ -0,0 +1,120 @@ +/* + * 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. + * + * Authors: + *Neil Roberts (nrobe...@igalia.com) + * + */ + +#include "nir.h" +#include "gl_nir.h" +#include "nir_builder.h" +#include "main/mtypes.h" + +/* + * This pass lowers the vulkan_resource_index intrinsic to a surface index. It + * is intended to be used with GL_ARB_gl_spirv. Unlike Vulkan, in that case it + * is not necessary to wait for the complete pipeline state to lower it. + */ + +static unsigned +find_block_by_binding(struct gl_linked_shader *linked_shader, + unsigned binding) +{ + unsigned num_blocks = linked_shader->Program->info.num_ubos; + struct gl_uniform_block **blocks = linked_shader->Program->sh.UniformBlocks; + + for (unsigned i = 0; i < num_blocks; i++) { + if (blocks[i]->Binding == binding) + return i; + } + + unreachable("No block found with the given binding"); +} + +static bool +convert_block(nir_block *block, + struct gl_linked_shader *linked_shader, + nir_builder *b) +{ + bool progress = false; + + nir_foreach_instr_safe(instr, block) { + if (instr->type != nir_instr_type_intrinsic) + continue; + + nir_intrinsic_instr *res_index = nir_instr_as_intrinsic(instr); + + if (res_index->intrinsic != nir_intrinsic_vulkan_resource_index) + continue; + + b->cursor = nir_after_instr(instr); + + /* The descriptor set should always be zero for GL */ + assert(nir_intrinsic_desc_set(res_index) == 0); +
[Mesa-dev] [PATCH 18/28] nir/linker: fill is_shader_storage for uniforms
--- src/compiler/glsl/gl_nir_link_uniforms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index 1a491dc2e5d..00995fb3f76 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -369,6 +369,8 @@ nir_link_uniform(struct gl_context *ctx, if (uniform->hidden) state->num_hidden_uniforms++; + uniform->is_shader_storage = nir_variable_is_in_ssbo(state->current_var); + /* @FIXME: the initialization of the following will be done as we * implement support for their specific features, like SSBO, atomics, * etc. @@ -379,7 +381,6 @@ nir_link_uniform(struct gl_context *ctx, uniform->array_stride = -1; uniform->row_major = false; uniform->builtin = false; - uniform->is_shader_storage = false; uniform->atomic_buffer_index = -1; uniform->top_level_array_size = 0; uniform->top_level_array_stride = 0; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 16/28] nir/linker: handle non-ubo uses of vulkan_resource_index
From: Neil Roberts In order to replicate the behaviour of lower_ubo_reference_visitor, the lowering code should search the list of blocks in ShaderStorageBlocks for the matching binding whenever a non-ubo usage of the resource index is encountered. The intended usage of the vulkan_resource_index is determined by searching for an intrinsic which uses the result. Unfortunately some other lower passes can add instructions to perform arithmetic on the result so the search needs to be performed recursively on the result of those. Signed-off-by: Neil Roberts Signed-off-by: Alejandro Piñeiro --- .../glsl/gl_nir_lower_vulkan_resource_index.c | 55 +++--- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c index 92ee3dd707a..561d2a03de2 100644 --- a/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c +++ b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c @@ -37,12 +37,10 @@ */ static unsigned -find_block_by_binding(struct gl_linked_shader *linked_shader, +find_block_by_binding(unsigned num_blocks, + struct gl_uniform_block **blocks, unsigned binding) { - unsigned num_blocks = linked_shader->Program->info.num_ubos; - struct gl_uniform_block **blocks = linked_shader->Program->sh.UniformBlocks; - for (unsigned i = 0; i < num_blocks; i++) { if (blocks[i]->Binding == binding) return i; @@ -51,6 +49,35 @@ find_block_by_binding(struct gl_linked_shader *linked_shader, unreachable("No block found with the given binding"); } +static bool +find_intrinsic_usage(nir_ssa_def *def, + bool *is_ubo_usage) +{ + nir_foreach_use_safe(use_src, def) { + if (use_src->parent_instr->type == nir_instr_type_alu) { + nir_alu_instr *alu = nir_instr_as_alu(use_src->parent_instr); + + if (find_intrinsic_usage(>dest.dest.ssa, is_ubo_usage)) +return true; + + continue; + } + + if (use_src->parent_instr->type != nir_instr_type_intrinsic) + continue; + + nir_intrinsic_instr *intr = nir_instr_as_intrinsic(use_src->parent_instr); + + if (intr == NULL) + continue; + + *is_ubo_usage = intr->intrinsic == nir_intrinsic_load_ubo; + return true; + } + + return false; +} + static bool convert_block(nir_block *block, struct gl_linked_shader *linked_shader, @@ -67,13 +94,29 @@ convert_block(nir_block *block, if (res_index->intrinsic != nir_intrinsic_vulkan_resource_index) continue; + bool is_ubo_usage; + if (!find_intrinsic_usage(_index->dest.ssa, _ubo_usage)) + continue; + b->cursor = nir_after_instr(instr); /* The descriptor set should always be zero for GL */ assert(nir_intrinsic_desc_set(res_index) == 0); - unsigned binding = nir_intrinsic_binding(res_index); - unsigned block = find_block_by_binding(linked_shader, binding); + + unsigned num_blocks; + struct gl_uniform_block **blocks; + + if (is_ubo_usage) { + num_blocks = linked_shader->Program->info.num_ubos; + blocks = linked_shader->Program->sh.UniformBlocks; + } else { + num_blocks = linked_shader->Program->info.num_ssbos; + blocks = linked_shader->Program->sh.ShaderStorageBlocks; + } + + unsigned block = find_block_by_binding(num_blocks, blocks, binding); + nir_ssa_def *surface = nir_iadd(b, nir_imm_int(b, block), -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 14/28] nir: add is_in_ubo/ssbo/block helpers
Equivalent to the already existing ir_variable is_in_buffer_block and is_in_shader_storage_block, adding the uniform buffer object one. I'm using the short forms (ssbo, ubo) to avoid having method names too long. --- src/compiler/nir/nir.h | 22 ++ 1 file changed, 22 insertions(+) diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index 5b871812d46..269eb47103c 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -3099,6 +3099,28 @@ uint64_t nir_get_single_slot_attribs_mask(uint64_t attribs, uint64_t dual_slot); nir_intrinsic_op nir_intrinsic_from_system_value(gl_system_value val); gl_system_value nir_system_value_from_intrinsic(nir_intrinsic_op intrin); + +static inline bool +nir_variable_is_in_ubo(const nir_variable *var) +{ + return (var->data.mode == nir_var_uniform && + var->interface_type != NULL); +} + +static inline bool +nir_variable_is_in_ssbo(const nir_variable *var) +{ + return (var->data.mode == nir_var_shader_storage && + var->interface_type != NULL); +} + +static inline bool +nir_variable_is_in_block(const nir_variable *var) +{ + return nir_variable_is_in_ubo(var) || nir_variable_is_in_ssbo(var); +} + + #ifdef __cplusplus } /* extern "C" */ #endif -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 17/28] nir/linker: add gl_nir_link_uniform_blocks.c
Adding the ability to link uniform blocks and shader storage blocks using NIR, intended for ARB_gl_spirv support. Among other things, this linking needs to take into account that everything should work without names, as they could be not present, while the GLSL IR uniform block linking was wrote with the names on its core. The other major difference compared with the GLSL IR linker is that we don't deal with layouts. There are no references to std140, std430, etc. Layouts are expressed through explicit offset, array stride and matrix stride. That simplifies how the buffer size are computed. But also means that we can't use the existing methods at glsl_types, so it is mostly computed here. This code only exposes the method gl_nir_link_uniform_blocks on gl_nir_linker.h It is worth to note that this linking do a iteration over the glsl_types, similarly to what the uniform linking do. A possible future improvement would be refactor both cases to try to share more code that it sharing right now. On GLSL IR there are a class visitor, specialized on each case, for that sharing. As adding a class visitor on C would more complicated, for now we are just iterating on both. Signed-off-by: Alejandro Piñeiro Signed-off-by: Neil Roberts --- src/compiler/Makefile.sources | 1 + src/compiler/glsl/gl_nir_link_uniform_blocks.c | 713 + src/compiler/glsl/gl_nir_linker.h | 3 + src/compiler/glsl/meson.build | 1 + 4 files changed, 718 insertions(+) create mode 100644 src/compiler/glsl/gl_nir_link_uniform_blocks.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index 3021bede6cf..df75109d120 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -30,6 +30,7 @@ LIBGLSL_FILES = \ glsl/gl_nir_lower_samplers_as_deref.c \ glsl/gl_nir_lower_vulkan_resource_index.c \ glsl/gl_nir_link_atomics.c \ + glsl/gl_nir_link_uniform_blocks.c \ glsl/gl_nir_link_uniform_initializers.c \ glsl/gl_nir_link_uniforms.c \ glsl/gl_nir_link_xfb.c \ diff --git a/src/compiler/glsl/gl_nir_link_uniform_blocks.c b/src/compiler/glsl/gl_nir_link_uniform_blocks.c new file mode 100644 index 000..8dd0bb6f71f --- /dev/null +++ b/src/compiler/glsl/gl_nir_link_uniform_blocks.c @@ -0,0 +1,713 @@ +/* + * Copyright © 2017 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.h" +#include "gl_nir_linker.h" +#include "ir_uniform.h" /* for gl_uniform_storage */ +#include "linker_util.h" +#include "main/shaderobj.h" /* _mesa_delete_linked_shader */ +#include "main/mtypes.h" + +/* Summary: This file contains code to do a nir-based linking for uniform + * blocks. This includes ubos and ssbos. + * + * More details: + * + * 1. Note that it is tailored to ARB_gl_spirv needs. Uniform block name, + * fields names, and other names are considered optional debug infor so could + * not be present. So the linking should work without it, and it is optional + * to not handle them at all. From ARB_gl_spirv: + * + *"19. How should the program interface query operations behave for program + * objects created from SPIR-V shaders? + * + * DISCUSSION: we previously said we didn't need reflection to work for + * SPIR-V shaders (at least for the first version), however we are left + * with specifying how it should "not work". The primary issue is that + * SPIR-V binaries are not required to have names associated with + * variables. They can be associated in debug information, but there is no + * requirement for that to be present, and it should not be relied upon. + * + * Options: + * + * + * + *C) Allow as much as possible to work "natur
[Mesa-dev] [PATCH 13/28] spirv/nir: fill glsl_type array stride
We need all the info when asking for the type, so we needed to call type_decoration_cb earlier, in order to get the ArrayStride. It is somewhat ugly to do this only for Array types, but we can't do it before the switch as type_decoration_cb have some asserts to ensure that the type and the decoration are compatible. One alternative would be keep the call to type_decoration_cb at the end, but create the glsl type for Arrays at the end, after calling it. Again we are treating Arrays in a different way. A full alternative to treat all types in the same way would be have a first switch(opcode) that would fill the base_type, call type_decoration_cb, and then a new switch(opcode) that would fill extra data and create the glsl_type. That looks like an overkill though. --- src/compiler/spirv/spirv_to_nir.c | 15 --- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 1201143d2f4..312d7d286ba 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -1143,9 +1143,14 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode, } val->type->base_type = vtn_base_type_array; - val->type->type = glsl_array_type(array_element->type, val->type->length); + /* We need to call type_decoration_cb earlier, in order to get the + * proper value of ArrayStride + */ + vtn_foreach_decoration(b, val, type_decoration_cb, NULL); + + val->type->type = glsl_full_array_type(array_element->type, val->type->length, + val->type->stride); val->type->array_element = array_element; - val->type->stride = 0; break; } @@ -1324,7 +1329,11 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode, vtn_fail("Unhandled opcode"); } - vtn_foreach_decoration(b, val, type_decoration_cb, NULL); + /* For Arrays we already called foreach_decoration */ + if (opcode != SpvOpTypeRuntimeArray && opcode != SpvOpTypeArray) { + vtn_foreach_decoration(b, val, type_decoration_cb, NULL); + } + } static nir_constant * -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 08/28] spirv/nir: Handle location decorations on block interface members
From: Neil Roberts Previously the code was taking any location decoration on the block and using that to calculate the member locations for all of the members. I think this was assuming that there would only be one location decoration for the entire block. According to the Vulkan spec it is possible to add location decorations to individual members: “If the structure type is a Block but without a Location, then each of its members must have a Location decoration. If it is a Block with a Location decoration, then its members are assigned consecutive locations in declaration order, starting from the first member which is initially the Block. Any member with its own Location decoration is assigned that location. Each remaining member is assigned the location after the immediately preceding member in declaration order.” This patch makes it instead keep track of which members have been assigned an explicit location. It also has a space to store the location for the struct as a whole. Once all the decorations have been processed it iterates over each member to fill in the missing locations using the rules described above. v2: update after commit b0c643d, where spirv_to_nir stopped to do struct member splitting, done it later in NIR (Alejandro Piñeiro) Signed-off-by: Neil Roberts Signed-off-by: Alejandro Piñeiro --- src/compiler/spirv/vtn_private.h | 6 src/compiler/spirv/vtn_variables.c | 62 -- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/compiler/spirv/vtn_private.h b/src/compiler/spirv/vtn_private.h index da7a04ce59f..a64ab99c47d 100644 --- a/src/compiler/spirv/vtn_private.h +++ b/src/compiler/spirv/vtn_private.h @@ -479,6 +479,12 @@ struct vtn_variable { nir_variable *var; + /* If the variable is a struct with a location set on it then this will be +* stored here. This will be used to calculate locations for members that +* don’t have their own explicit location. +*/ + int base_location; + int shared_location; /** diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 9a4ddeaa822..2a7a5b4947c 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1443,13 +1443,11 @@ var_decoration_cb(struct vtn_builder *b, struct vtn_value *val, int member, */ if (dec->decoration == SpvDecorationLocation) { unsigned location = dec->literals[0]; - bool is_vertex_input = false; if (b->shader->info.stage == MESA_SHADER_FRAGMENT && vtn_var->mode == vtn_variable_mode_output) { location += FRAG_RESULT_DATA0; } else if (b->shader->info.stage == MESA_SHADER_VERTEX && vtn_var->mode == vtn_variable_mode_input) { - is_vertex_input = true; location += VERT_ATTRIB_GENERIC0; } else if (vtn_var->mode == vtn_variable_mode_input || vtn_var->mode == vtn_variable_mode_output) { @@ -1466,14 +1464,13 @@ var_decoration_cb(struct vtn_builder *b, struct vtn_value *val, int member, } else { /* This handles the structure member case */ assert(vtn_var->var->members); - for (unsigned i = 0; i < vtn_var->var->num_members; i++) { -vtn_var->var->members[i].location = location; -const struct glsl_type *member_type = - glsl_get_struct_field(vtn_var->var->interface_type, i); -location += glsl_count_attribute_slots(member_type, - is_vertex_input); - } + + if (member == -1) +vtn_var->base_location = location; + else +vtn_var->var->members[member].location = location; } + return; } else { if (vtn_var->var) { @@ -1666,6 +1663,43 @@ is_per_vertex_inout(const struct vtn_variable *var, gl_shader_stage stage) return false; } +static void +add_missing_member_locations(struct vtn_variable *var, + bool is_vertex_input) +{ + unsigned length = + glsl_get_length(glsl_without_array(var->type->type)); + int location = var->base_location; + + for (unsigned i = 0; i < length; i++) { + /* From the Vulkan spec: + * + * “If the structure type is a Block but without a Location, then each + * of its members must have a Location decoration.” + */ + assert(var->base_location != -1 || + var->var->members[i].location != -1); + + /* From the Vulkan spec: + * + * “Any member with its own Location decoration is assigned that + * location. Each remaining member is assigned the location after the + * immediately preceding member in declaration order.” + */ + if (var->var->members[i].location != -1) +
[Mesa-dev] [PATCH 09/28] nir/types: add three new wrapper helpers
To already existing fields on glsl_types. Specifically: * glsl_get_struct_field_offset * glsl_get_struct_field_matrix_layout * glsl_type_arrays_of_arrays_size --- src/compiler/nir_types.cpp | 21 + src/compiler/nir_types.h | 8 2 files changed, 29 insertions(+) diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp index d24f0941519..2a1ae42a9bb 100644 --- a/src/compiler/nir_types.cpp +++ b/src/compiler/nir_types.cpp @@ -72,6 +72,21 @@ glsl_get_struct_field(const glsl_type *type, unsigned index) return type->fields.structure[index].type; } +const int +glsl_get_struct_field_offset(const struct glsl_type *type, + unsigned index) +{ + return type->fields.structure[index].offset; +} + +const unsigned +glsl_get_struct_field_matrix_layout(const struct glsl_type *type, +unsigned index) +{ + return type->fields.structure[index].matrix_layout; +} + + const glsl_type * glsl_get_function_return_type(const glsl_type *type) { @@ -591,3 +606,9 @@ glsl_contains_atomic(const struct glsl_type *type) { return type->contains_atomic(); } + +unsigned +glsl_type_arrays_of_arrays_size(const struct glsl_type *type) +{ + return type->arrays_of_arrays_size(); +} diff --git a/src/compiler/nir_types.h b/src/compiler/nir_types.h index 77454fa9fab..69de44c3423 100644 --- a/src/compiler/nir_types.h +++ b/src/compiler/nir_types.h @@ -46,6 +46,11 @@ const char *glsl_get_type_name(const struct glsl_type *type); const struct glsl_type *glsl_get_struct_field(const struct glsl_type *type, unsigned index); +const int glsl_get_struct_field_offset(const struct glsl_type *type, + unsigned index); + +const unsigned glsl_get_struct_field_matrix_layout(const struct glsl_type *type, + unsigned index); const struct glsl_type *glsl_get_array_element(const struct glsl_type *type); const struct glsl_type *glsl_without_array(const struct glsl_type *type); const struct glsl_type *glsl_without_array_or_matrix(const struct glsl_type *type); @@ -91,6 +96,9 @@ unsigned glsl_get_record_location_offset(const struct glsl_type *type, unsigned glsl_atomic_size(const struct glsl_type *type); + +unsigned glsl_type_arrays_of_arrays_size(const struct glsl_type *type); + static inline unsigned glsl_get_bit_size(const struct glsl_type *type) { -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 02/28] spirv/nir: translate ssbo
They are supported by SPIR-V for OpenGL. OpenGL codepath expect nir to include the ssbo as nir variables. --- src/compiler/spirv/vtn_variables.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 3eb1e4e9c97..5665106ab14 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1519,7 +1519,7 @@ vtn_storage_class_to_mode(struct vtn_builder *b, nir_mode = nir_var_uniform; } else if (interface_type->buffer_block) { mode = vtn_variable_mode_ssbo; - nir_mode = 0; + nir_mode = nir_var_shader_storage; } else { /* Default-block uniforms, coming from gl_spirv */ mode = vtn_variable_mode_uniform; @@ -1715,6 +1715,7 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, case vtn_variable_mode_global: case vtn_variable_mode_uniform: case vtn_variable_mode_ubo: + case vtn_variable_mode_ssbo: /* For these, we create the variable normally */ var->var = rzalloc(b->shader, nir_variable); var->var->name = ralloc_strdup(var->var, val->name); @@ -1819,7 +1820,6 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, break; } - case vtn_variable_mode_ssbo: case vtn_variable_mode_push_constant: /* These don't need actual variables. */ break; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 05/28] spirv/nir: include SPIR-V explicit offset on the glsl struct type
From ARB_gl_spirv spec: "Mapping of layouts std140/std430 -> explicit *Offset*, *ArrayStride*, and *MatrixStride* Decoration on struct members" and "A variable in the *Uniform* Storage Class decorated as a *Block* must be explicitly laid out using the *Offset*, *ArrayStride*, and *MatrixStride* decorations. If the variable is decorated as a *BufferBlock*, its offsets and strides must not contradict std430 alignment and minimum offset requirements." So for uniform blocks, we need the explicit offset coming from the SPIR-V shader. --- src/compiler/spirv/spirv_to_nir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 37a801037b9..28f4716b40e 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -734,7 +734,7 @@ struct_member_decoration_cb(struct vtn_builder *b, ctx->type->builtin_block = true; break; case SpvDecorationOffset: - ctx->type->offsets[member] = dec->literals[0]; + ctx->type->offsets[member] = ctx->fields[member].offset = dec->literals[0]; break; case SpvDecorationMatrixStride: /* Handled as a second pass */ -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 06/28] spirv/nir: include row major coming from SPIR-V on the glsl type
--- src/compiler/spirv/spirv_to_nir.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 28f4716b40e..52c3c968bb7 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -743,6 +743,7 @@ struct_member_decoration_cb(struct vtn_builder *b, break; /* Nothing to do here. Column-major is the default. */ case SpvDecorationRowMajor: mutable_matrix_member(b, ctx->type, member)->row_major = true; + ctx->fields[member].matrix_layout = GLSL_MATRIX_LAYOUT_ROW_MAJOR; break; case SpvDecorationPatch: -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 04/28] spirv/nir: fill up nir variable info for ubos and ssbo
Some nir variables are only filled up for some specific modes. We found to need the binding for ubos/ssbos. The comment before that code (starts with XXX) points that binding still needs to be filled up for uniform variables at that point, and that should be fixed, although it doesn't specify why that's a problem or the alternative. For now doing the same for ubos/ssbos, and will hope that the future fixing is done for all of them. --- src/compiler/spirv/vtn_variables.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 04103455614..957ef0610b7 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1841,7 +1841,9 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, vtn_foreach_decoration(b, val, var_decoration_cb, var); - if (var->mode == vtn_variable_mode_uniform) { + if (var->mode == vtn_variable_mode_uniform || + var->mode == vtn_variable_mode_ubo || + var->mode == vtn_variable_mode_ssbo) { /* XXX: We still need the binding information in the nir_variable * for these. We should fix that. */ -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 00/28 v3] ARB_gl_spirv: v3 ubo/ssbo support, plus CTS goodness
This is the third version of the ubo/ssbo support for ARB_gl_spirv. It is mostly a v2 resend, rebased against master, and small tweaks to fix minor rebase conflicts. Just to remember: * AOA of ubo/ssbo not included. Will be supported on a future series. * This series include two patches at the end, not really related with ubo/ssbo support. Just included on this series to get all the ARB_gl_spirv CTS tests passing. Patches can be found here: https://github.com/Igalia/mesa/tree/arb_gl_spirv-series6-ubo-ssbo-v3 And can be tested with: https://github.com/Igalia/piglit/tree/arb_gl_spirv-series5-ubo-ssbo-v3 Thanks in advance. Alejandro Piñeiro (23): spirv/nir: translate uniform blocks spirv/nir: translate ssbo spirv/nir: setting interface type for ubos/ssbos spirv/nir: fill up nir variable info for ubos and ssbo spirv/nir: include SPIR-V explicit offset on the glsl struct type spirv/nir: include row major coming from SPIR-V on the glsl type spirv/nir: don't set interface_type if it is not a struct nir/types: add three new wrapper helpers glsl_types/nir: add matrix_stride plus nir wrapper helpers spirv/nir: fill glsl_struct_field explicit_matrix_stride glsl_types/nir: add explicit_array_stride plus nir wrapper helpers spirv/nir: fill glsl_type array stride nir: add is_in_ubo/ssbo/block helpers nir/linker: add gl_nir_link_uniform_blocks.c nir/linker: fill is_shader_storage for uniforms nir/linker: use only the array element type for array of ssbo/ubo nir/linker: fill up uniform_storage with explicit data nir/linker: update already processed uniforms search for UBOs/SSBOs nir/linker: add program ubo/ssbo at the resource list i965: use GLboolean for all brw_link_shader returns i965: call to gl_nir_link_uniform_blocks mesa: add NULL name check for NUM_ACTIVE_VARIABLES query mesa: add NULL name check for several length queries Antia Puentes (2): nir/linker: Set the uniform's block_index nir/linker: Add inputs/outputs to the program resource list Neil Roberts (3): spirv/nir: Handle location decorations on block interface members nir/linker/i965: Lower vulkan_resource_index during linking nir/linker: handle non-ubo uses of vulkan_resource_index src/compiler/Makefile.sources | 2 + src/compiler/glsl/gl_nir.h | 4 + src/compiler/glsl/gl_nir_link_uniform_blocks.c | 713 + src/compiler/glsl/gl_nir_link_uniforms.c | 178 - src/compiler/glsl/gl_nir_linker.c | 93 +++ src/compiler/glsl/gl_nir_linker.h | 3 + src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +- .../glsl/gl_nir_lower_vulkan_resource_index.c | 163 + src/compiler/glsl/meson.build | 2 + src/compiler/glsl_types.cpp| 31 +- src/compiler/glsl_types.h | 23 +- src/compiler/nir/nir.h | 22 + src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 +- src/compiler/nir/nir_split_per_member_structs.c| 3 +- src/compiler/nir/nir_split_vars.c | 7 +- src/compiler/nir_types.cpp | 47 +- src/compiler/nir_types.h | 20 +- src/compiler/spirv/spirv_to_nir.c | 24 +- src/compiler/spirv/vtn_private.h | 6 + src/compiler/spirv/vtn_variables.c | 90 ++- src/mesa/drivers/dri/i965/brw_link.cpp | 12 +- src/mesa/main/shader_query.cpp | 42 +- src/mesa/main/shaderapi.c | 26 +- 23 files changed, 1433 insertions(+), 83 deletions(-) create mode 100644 src/compiler/glsl/gl_nir_link_uniform_blocks.c create mode 100644 src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 07/28] spirv/nir: don't set interface_type if it is not a struct
vnt_variables uses interface_type on several use cases, but on nir variable it is more limited. From nir.h: /** * For variables that are in an interface block or are an instance of an * interface block, this is the \c GLSL_TYPE_INTERFACE type for that block. * * \sa ir_variable::location */ But interface blocks expects the type to be an struct, so those cases should not be filled. For example, glsl checks if a variable is in an uniform block if it is an uniform and has an interface type. One example of why this is needed: gl_PatchVerticesIn is lowered to an uniform. Without this change, it would include a interface_type. Then, we would try to initialize the uniform block, and find that it doesn't have any component. --- src/compiler/spirv/vtn_variables.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 957ef0610b7..9a4ddeaa822 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1818,6 +1818,8 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, var->var->members[i].mode = nir_mode; var->var->members[i].patch = var->patch; } + } else { + var->var->interface_type = NULL; } /* For inputs and outputs, we need to grab locations and builtin -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 01/28] spirv/nir: translate uniform blocks
They are supported by SPIR-V for ARB_gl_spirv. --- src/compiler/spirv/vtn_variables.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index cc3438bff23..3eb1e4e9c97 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1516,7 +1516,7 @@ vtn_storage_class_to_mode(struct vtn_builder *b, case SpvStorageClassUniform: if (interface_type->block) { mode = vtn_variable_mode_ubo; - nir_mode = 0; + nir_mode = nir_var_uniform; } else if (interface_type->buffer_block) { mode = vtn_variable_mode_ssbo; nir_mode = 0; @@ -1714,6 +1714,7 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, case vtn_variable_mode_local: case vtn_variable_mode_global: case vtn_variable_mode_uniform: + case vtn_variable_mode_ubo: /* For these, we create the variable normally */ var->var = rzalloc(b->shader, nir_variable); var->var->name = ralloc_strdup(var->var, val->name); @@ -1818,7 +1819,6 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, break; } - case vtn_variable_mode_ubo: case vtn_variable_mode_ssbo: case vtn_variable_mode_push_constant: /* These don't need actual variables. */ -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 03/28] spirv/nir: setting interface type for ubos/ssbos
Right now, a type is considered a ubo/ssbo if the mode is uniform/shader_storage and the interface_type is different to NULL. See ir_variable::in_in_buffer_block as an example. --- src/compiler/spirv/vtn_variables.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index 5665106ab14..04103455614 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1731,7 +1731,16 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, } var->var->data.mode = nir_mode; var->var->data.location = -1; - var->var->interface_type = NULL; + + switch (var->mode) { + case vtn_variable_mode_ubo: + case vtn_variable_mode_ssbo: + var->var->interface_type = without_array->type; + break; + default: + var->var->interface_type = NULL; + break; + } break; case vtn_variable_mode_workgroup: -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/13] nir: Add a pass for gathering transform feedback info
Just in case you missed my Rb the first time you sent this patch: Reviewed-by: Alejandro Piñeiro On 13/10/18 15:09, 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. > --- > 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 000..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 > + > +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)) { > +
Re: [Mesa-dev] [PATCH] nir: Add a pass for gathering transform feedback info
After the experience of using it, and reading it, the patch LGTM. I still have some issues while trying to use this pass, but they are mostly glslang bugs, or things that I suspect is a problem on a different pass or on our linking code, that are better to talk in a different thread. So this patch: Reviewed-by: Alejandro Piñeiro 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. > --- > 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 000..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 > + > +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,
Re: [Mesa-dev] [PATCH] nir: Add a pass for gathering transform feedback info
I was not able to finish trying to get ARB_gl_spirv using this pass. The major difference is that on ARB_gl_spirv (and afaiu on GLSL too) we are merging the info of all the available xfb varyings from all the stages, while this pass gathers info from a individual nir shader (so one individual stage). Having said so, while using this pass, I found some issues/questions, see below inline. 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. > --- > 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 000..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 > + > +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); > +
Re: [Mesa-dev] [PATCH] nir: Add a pass for gathering transform feedback info
On 05/10/18 17:44, Jason Ekstrand wrote: > On Fri, Oct 5, 2018 at 10:34 AM Alejandro Piñeiro > mailto:apinhe...@igalia.com>> wrote: > > 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. > > > 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. Ok, then next week I will check for sure if we can do that. > If we do go that route, however, we may want to do something better > than assert() for the error handling. 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. > > > > --- > > 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 000..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 t
Re: [Mesa-dev] [PATCH] nir: Add a pass for gathering transform feedback info
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. > --- > 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 000..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 > + > +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 { > +
Re: [Mesa-dev] [PATCH 1/5] glspirv: drop pointless assert (size_t is unsigned)
Reviewed-by: Alejandro Piñeiro On 05/10/18 02:00, Dave Airlie wrote: > From: Dave Airlie > > Found by coverity > --- > src/mesa/main/glspirv.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/src/mesa/main/glspirv.c b/src/mesa/main/glspirv.c > index fecf7384eb3..972989055e9 100644 > --- a/src/mesa/main/glspirv.c > +++ b/src/mesa/main/glspirv.c > @@ -73,8 +73,6 @@ _mesa_spirv_shader_binary(struct gl_context *ctx, > struct gl_spirv_module *module; > struct gl_shader_spirv_data *spirv_data; > > - assert(length >= 0); > - > module = malloc(sizeof(*module) + length); > if (!module) { >_mesa_error(ctx, GL_OUT_OF_MEMORY, "glShaderBinary"); ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] glsl_types/nir: add explicit_array_stride plus nir wrapper helpers
From ARB_gl_spirv: "Mapping of layouts std140/std430 -> explicit *Offset*, *ArrayStride*, and *MatrixStride* Decoration on struct members" That means that we would not have available any kind of layout info, and we should use explicit array strides. This commit adds explicit_array_stride. The default value is -1 meaning that it is not set (as with offset). That should be the default value for GLSL. In general, the default constructor is ok. We just need to be careful with some array lowerings, as it should try to get the explicit array stride when creating new types. Note that this means that for the ARB_gl_spirv case std430_array_stride, std140_size etc are meaningless (unless you guess the layout, something that you shouldn't). v2: add missing glsl_full_array_type call, found while testing ARB_gl_spirv with borrowed tests (Alejandro) --- src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +- src/compiler/glsl_types.cpp| 28 +- src/compiler/glsl_types.h | 13 +++--- src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 ++- src/compiler/nir/nir_split_per_member_structs.c| 3 ++- src/compiler/nir/nir_split_vars.c | 7 -- src/compiler/nir_types.cpp | 20 +--- src/compiler/nir_types.h | 10 +++- src/compiler/spirv/vtn_variables.c | 3 ++- 9 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c b/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c index 9ff5708f503..9716ac4562a 100644 --- a/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c +++ b/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c @@ -99,7 +99,7 @@ remove_struct_derefs_prep(nir_deref_instr **p, char **name, remove_struct_derefs_prep([1], name, location, type); - *type = glsl_get_array_instance(*type, length); + *type = glsl_get_array_instance(*type, length, glsl_get_explicit_array_stride(cur->type)); break; } diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index ed3bb3a9889..1326b21913b 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -44,7 +44,7 @@ glsl_type::glsl_type(GLenum gl_type, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(vector_elements), matrix_columns(matrix_columns), - length(0) + length(0), explicit_array_stride(-1) { /* Values of these types must fit in the two bits of * glsl_type::sampled_type. @@ -77,7 +77,7 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, base_type(base_type), sampled_type(type), sampler_dimensionality(dim), sampler_shadow(shadow), sampler_array(array), interface_packing(0), - interface_row_major(0), length(0) + interface_row_major(0), length(0), explicit_array_stride(-1) { this->mem_ctx = ralloc_context(NULL); assert(this->mem_ctx != NULL); @@ -97,7 +97,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(num_fields) + length(num_fields), explicit_array_stride(-1) { unsigned int i; @@ -127,7 +127,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, interface_packing((unsigned) packing), interface_row_major((unsigned) row_major), vector_elements(0), matrix_columns(0), - length(num_fields) + length(num_fields), explicit_array_stride(-1) { unsigned int i; @@ -152,7 +152,7 @@ glsl_type::glsl_type(const glsl_type *return_type, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(num_params) + length(num_params), explicit_array_stride(-1) { unsigned int i; @@ -181,7 +181,7 @@ glsl_type::glsl_type(const char *subroutine_name) : sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(1), matrix_columns(1), - length(0) + length(0), explicit_array_stride(-1) { this->mem_ctx = ralloc_context(NULL); assert(this->mem_ctx != NULL); @@ -434,12 +434,12 @@ _mesa_glsl_release_types(void) } -glsl_type::glsl_type(const glsl_type *array, unsigned length) : +glsl_type::glsl_type(const glsl_type *array, unsigned length, int explicit_array_stride) : base_type(GLSL_TYPE_ARRAY), sampled_type(GLSL_TYPE_VOID), sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(length), name(NULL) + length(length),
[Mesa-dev] [PATCH v2 26/28] mesa: add NULL name check for NUM_ACTIVE_VARIABLES query
This can happens if we are running an SPIR-V shader (ARB_gl_spirv). --- src/mesa/main/shader_query.cpp | 30 -- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index 11ecd71c575..b775b4231c2 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -1013,11 +1013,16 @@ get_buffer_property(struct gl_shader_program *shProg, *val = 0; for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms; i++) { const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName; -struct gl_program_resource *uni = - _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname, -NULL); -if (!uni) - continue; +/* IndexName can be NULL if we are using a SPIR-V shader + * (ARB_gl_spirv). + */ +if (iname != NULL) { + struct gl_program_resource *uni = + _mesa_program_resource_find_name(shProg, GL_UNIFORM, iname, + NULL); + if (!uni) + continue; +} (*val)++; } return 1; @@ -1049,11 +1054,16 @@ get_buffer_property(struct gl_shader_program *shProg, *val = 0; for (unsigned i = 0; i < RESOURCE_UBO(res)->NumUniforms; i++) { const char *iname = RESOURCE_UBO(res)->Uniforms[i].IndexName; -struct gl_program_resource *uni = - _mesa_program_resource_find_name(shProg, GL_BUFFER_VARIABLE, -iname, NULL); -if (!uni) - continue; +/* IndexName can be NULL if we are using a SPIR-V shader + * (ARB_gl_spirv). + */ +if (iname != NULL) { + struct gl_program_resource *uni = + _mesa_program_resource_find_name(shProg, GL_BUFFER_VARIABLE, + iname, NULL); + if (!uni) + continue; +} (*val)++; } return 1; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 28/28] nir/linker: Add inputs/outputs to the program resource list
From: Antia Puentes --- src/compiler/glsl/gl_nir_linker.c | 79 +++ 1 file changed, 79 insertions(+) diff --git a/src/compiler/glsl/gl_nir_linker.c b/src/compiler/glsl/gl_nir_linker.c index 138a12e532d..acec0fe1f03 100644 --- a/src/compiler/glsl/gl_nir_linker.c +++ b/src/compiler/glsl/gl_nir_linker.c @@ -33,6 +33,58 @@ * Also note that this is tailored for ARB_gl_spirv needs and particularities */ +static bool +add_interface_variables(const struct gl_context *cts, +struct gl_shader_program *prog, +struct set *resource_set, +unsigned stage, GLenum programInterface) +{ + const struct exec_list *var_list = NULL; + + struct gl_linked_shader *sh = prog->_LinkedShaders[stage]; + if (!sh) + return true; + + nir_shader *nir = sh->Program->nir; + assert(nir); + + switch (programInterface) { + case GL_PROGRAM_INPUT: + var_list = >inputs; + break; + case GL_PROGRAM_OUTPUT: + var_list = >outputs; + break; + default: + assert("!Should not get here"); + break; + } + + nir_foreach_variable(var, var_list) { + if (var->data.how_declared == nir_var_hidden) + continue; + + struct gl_shader_variable *sh_var = + rzalloc(prog, struct gl_shader_variable); + + /* In the ARB_gl_spirv spec, names are considered optional debug info, so + * the linker needs to work without them. Returning them is optional. + * For simplicity, we ignore names. + */ + sh_var->name = NULL; + sh_var->type = var->type; + sh_var->location = var->data.location; + + if (!link_util_add_program_resource(prog, resource_set, + programInterface, + sh_var, 1 << stage)) { + return false; + } + } + + return true; +} + void nir_build_program_resource_list(struct gl_context *ctx, struct gl_shader_program *prog) @@ -44,10 +96,37 @@ nir_build_program_resource_list(struct gl_context *ctx, prog->data->NumProgramResourceList = 0; } + int input_stage = MESA_SHADER_STAGES, output_stage = 0; + + /* Determine first input and final output stage. These are used to +* detect which variables should be enumerated in the resource list +* for GL_PROGRAM_INPUT and GL_PROGRAM_OUTPUT. +*/ + for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) { + if (!prog->_LinkedShaders[i]) + continue; + if (input_stage == MESA_SHADER_STAGES) + input_stage = i; + output_stage = i; + } + + /* Empty shader, no resources. */ + if (input_stage == MESA_SHADER_STAGES && output_stage == 0) + return; + struct set *resource_set = _mesa_set_create(NULL, _mesa_hash_pointer, _mesa_key_pointer_equal); + /* Add inputs and outputs to the resource list. */ + if (!add_interface_variables(ctx, prog, resource_set, input_stage, +GL_PROGRAM_INPUT)) + return; + + if (!add_interface_variables(ctx, prog, resource_set, output_stage, +GL_PROGRAM_OUTPUT)) + return; + /* Add uniforms * * Here, it is expected that nir_link_uniforms() has already been -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 27/28] mesa: add NULL name check for several length queries
Since ARB_gl_spirv it is possible to miss a lot of name reflection information, so it is needed to add NULL name checks for several queries, and return a specific value on those cases. This commit add them for ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH, ACTIVE_ATTRIBUTE_MAX_LENGTH and ACTIVE_UNIFORM_MAX_LENGTH. From ARB_gl_spirv spec: "If pname is ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH, the length of the longest active uniform block name, including the null terminator, is returned. If no active uniform blocks exist, zero is returned. If no name reflection information is available, one is returned. If pname is ACTIVE_ATTRIBUTE_MAX_LENGTH, the length of the longest active attribute name, including a null terminator, is returned. If no active attributes exist, zero is returned. If no name reflection information is available, one is returned. If pname is ACTIVE_UNIFORM_MAX_LENGTH, the length of the longest active uniform name, including a null terminator, is returned. If no active uniforms exist, zero is returned. If no name reflection information is available, one is returned." --- src/mesa/main/shader_query.cpp | 12 ++-- src/mesa/main/shaderapi.c | 26 ++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index b775b4231c2..0a85e183a0c 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -244,9 +244,17 @@ _mesa_longest_attribute_name_length(struct gl_shader_program *shProg) if (res->Type == GL_PROGRAM_INPUT && res->StageReferences & (1 << MESA_SHADER_VERTEX)) { - const size_t length = strlen(RESOURCE_VAR(res)->name); + /* From ARB_gl_spirv spec: + * "If pname is ACTIVE_ATTRIBUTE_MAX_LENGTH, the length of the + *longest active attribute name, including a null terminator, is + *returned. If no active attributes exist, zero is returned. If + *no name reflection information is available, one is returned." + */ + const size_t length = RESOURCE_VAR(res)->name != NULL ? + strlen(RESOURCE_VAR(res)->name) : 1; + if (length >= longest) - longest = length + 1; + longest = RESOURCE_VAR(res)->name != NULL ? length + 1 : length; } } diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c index 2ea8d965aba..3e532c1b41e 100644 --- a/src/mesa/main/shaderapi.c +++ b/src/mesa/main/shaderapi.c @@ -728,11 +728,22 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname, if (shProg->data->UniformStorage[i].is_shader_storage) continue; + /* From ARB_gl_spirv spec: + * "If pname is ACTIVE_UNIFORM_MAX_LENGTH, the length of the + *longest active uniform name, including a null terminator, is + *returned. If no active uniforms exist, zero is returned. If no + *name reflection information is available, one is returned." + * + * We are setting 0 here, as below it will add 1 for the NUL character. + */ + const GLint base_len = shProg->data->UniformStorage[i].name != NULL ? +strlen(shProg->data->UniformStorage[i].name) : 0; + /* Add one for the terminating NUL character for a non-array, and * 4 for the "[0]" and the NUL for an array. */ - const GLint len = strlen(shProg->data->UniformStorage[i].name) + 1 + - ((shProg->data->UniformStorage[i].array_elements != 0) ? 3 : 0); + const GLint len = base_len + 1 + +((shProg->data->UniformStorage[i].array_elements != 0) ? 3 : 0); if (len > max_len) max_len = len; @@ -810,9 +821,16 @@ get_programiv(struct gl_context *ctx, GLuint program, GLenum pname, break; for (i = 0; i < shProg->data->NumUniformBlocks; i++) { -/* Add one for the terminating NUL character. +/* Add one for the terminating NUL character. Name can be NULL, in + * that case, from ARB_gl_spirv: + * "If pname is ACTIVE_UNIFORM_BLOCK_MAX_NAME_LENGTH, the length of + *the longest active uniform block name, including the null + *terminator, is returned. If no active uniform blocks exist, + *zero is returned. If no name reflection information is + *available, one is returned." */ - const GLint len = strlen(shProg->data->UniformBlocks[i].Name) + 1; + const GLint len = shProg->data->UniformBlocks[i].Name ? +strlen(shProg->data->UniformBlocks[i].Name) + 1 : 1; if (len > max_len) max_len = len; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 22/28] nir/linker: Set the uniform's block_index
From: Antia Puentes Binding comparison is used to determine the block the uniform is part of. To do the binding comparison we need the information in UniformBlocks[] and ShaderStorageBlocks[] to be available, so we have to call gl_nir_link_uniform_blocks() before linking the uniforms. --- src/compiler/glsl/gl_nir_link_uniforms.c | 22 +- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index d266091ba80..77def1a623f 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -454,11 +454,31 @@ nir_link_uniform(struct gl_context *ctx, else uniform->offset = 0; + int buffer_block_index = -1; + /* If the uniform is inside a uniform block determine its block index by + * comparing the bindings, we can not use names. + */ + if (nir_variable_is_in_block(state->current_var)) { + struct gl_uniform_block *blocks = nir_variable_is_in_ssbo(state->current_var) ? +prog->data->ShaderStorageBlocks : prog->data->UniformBlocks; + + int num_blocks = nir_variable_is_in_ssbo(state->current_var) ? +prog->data->NumShaderStorageBlocks : prog->data->NumUniformBlocks; + + for (unsigned i = 0; i < num_blocks; i++) { +if (state->current_var->data.binding == blocks[i].Binding) { + buffer_block_index = i; +} + } + assert(buffer_block_index >= 0); + } + + uniform->block_index = buffer_block_index; + /* @FIXME: the initialization of the following will be done as we * implement support for their specific features, like SSBO, atomics, * etc. */ - uniform->block_index = -1; uniform->builtin = false; uniform->atomic_buffer_index = -1; uniform->top_level_array_size = 0; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 24/28] i965: use GLboolean for all brw_link_shader returns
The function had a mix of true/GL_TRUE and false/GL_FALSE returns. Using GL_TRUE/GL_FALSE as the function returns a GLboolean. --- src/mesa/drivers/dri/i965/brw_link.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp index 37b775637b4..03b32d1fe7a 100644 --- a/src/mesa/drivers/dri/i965/brw_link.cpp +++ b/src/mesa/drivers/dri/i965/brw_link.cpp @@ -264,7 +264,7 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) /* SPIR-V programs use a NIR linker */ if (shProg->data->spirv) { if (!gl_nir_link_uniforms(ctx, shProg)) - return false; + return GL_FALSE; gl_nir_link_assign_atomic_counter_resources(ctx, shProg); gl_nir_link_assign_xfb_resources(ctx, shProg); @@ -375,7 +375,7 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) } if (brw->precompile && !brw_shader_precompile(ctx, shProg)) - return false; + return GL_FALSE; /* SPIR-V programs build its resource list from linked NIR shaders. */ if (!shProg->data->spirv) @@ -393,5 +393,5 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) shader->ir = NULL; } - return true; + return GL_TRUE; } -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 25/28] i965: call to gl_nir_link_uniform_blocks
When using a SPIR-V shader. Note that needs to be done before linking uniforms, so when creating the uniform storage entries, block_index could be filled properly (among other things). --- src/mesa/drivers/dri/i965/brw_link.cpp | 4 1 file changed, 4 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_link.cpp b/src/mesa/drivers/dri/i965/brw_link.cpp index 03b32d1fe7a..d0179cc89a1 100644 --- a/src/mesa/drivers/dri/i965/brw_link.cpp +++ b/src/mesa/drivers/dri/i965/brw_link.cpp @@ -263,6 +263,10 @@ brw_link_shader(struct gl_context *ctx, struct gl_shader_program *shProg) /* SPIR-V programs use a NIR linker */ if (shProg->data->spirv) { + if (!gl_nir_link_uniform_blocks(ctx, shProg)) { + return GL_FALSE; + } + if (!gl_nir_link_uniforms(ctx, shProg)) return GL_FALSE; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 23/28] nir/linker: add program ubo/ssbo at the resource list
--- src/compiler/glsl/gl_nir_linker.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/src/compiler/glsl/gl_nir_linker.c b/src/compiler/glsl/gl_nir_linker.c index 547549bc4e0..138a12e532d 100644 --- a/src/compiler/glsl/gl_nir_linker.c +++ b/src/compiler/glsl/gl_nir_linker.c @@ -67,5 +67,19 @@ nir_build_program_resource_list(struct gl_context *ctx, } + /* Add program uniform blocks. */ + for (unsigned i = 0; i < prog->data->NumUniformBlocks; i++) { + if (!link_util_add_program_resource(prog, resource_set, GL_UNIFORM_BLOCK, + >data->UniformBlocks[i], 0)) + return; + } + + /* Add program shader storage blocks. */ + for (unsigned i = 0; i < prog->data->NumShaderStorageBlocks; i++) { + if (!link_util_add_program_resource(prog, resource_set, GL_SHADER_STORAGE_BLOCK, + >data->ShaderStorageBlocks[i], 0)) + return; + } + _mesa_set_destroy(resource_set, NULL); } -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 21/28] nir/linker: update already processed uniforms search for UBOs/SSBOs
Until now, we were using the uniform explicit location to check if the current nir variable already was processed, and entries on the uniform storage added. But for UBOs/SSBOs, entries are added but we lack a explicit location. For those we need to rely on the UBO/SSBO binding (to the nir variable binding, and the uniform storage block_index). In that case several uniforms would need to be updated at once. --- src/compiler/glsl/gl_nir_link_uniforms.c | 78 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index 448f8277c16..d266091ba80 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -130,20 +130,79 @@ nir_setup_uniform_remap_tables(struct gl_context *ctx, } } +static void +update_uniform_storage(struct gl_uniform_storage *uniform, + unsigned stage) +{ + uniform->active_shader_mask |= 1 << stage; +} + +/** + * Finds, return, and update the stage infor for any uniform at the + * UniformStorage any uniform defined by @var. In general this is done using + * the explicit location, except: + * + * * UBOs/SSBOs: as they lack explicit location, binding is used to locate + * them. That means that more that one entry at the uniform storage can be + * found. In that case all of them are updated, and the first entry is + * returned, in order to update the location of nir variable. + * + * * Expecial uniforms: like atomic counters. They lack a explicit location, + * so they are skipped, handled in any case, and assign a location later. + * + */ static struct gl_uniform_storage * -find_previous_uniform_storage(struct gl_shader_program *prog, - int location) +find_and_update_previous_uniform_storage(struct gl_shader_program *prog, + nir_variable *var, + unsigned stage) { - /* This would only work for uniform with explicit location, as all the -* uniforms without location (ie: atomic counters) would have a initial -* location equal to -1. We early return in that case. + if (nir_variable_is_in_block(var)) { + struct gl_uniform_storage *uniform = NULL; + + unsigned num_blks = nir_variable_is_in_ubo(var) ? + prog->data->NumUniformBlocks : + prog->data->NumShaderStorageBlocks; + + struct gl_uniform_block *blks = nir_variable_is_in_ubo(var) ? + prog->data->UniformBlocks : prog->data->ShaderStorageBlocks; + + for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) { + /* UniformStorage contains both variables from ubos and ssbos */ + if ( prog->data->UniformStorage[i].is_shader_storage != + nir_variable_is_in_ssbo(var)) +continue; + + int block_index = prog->data->UniformStorage[i].block_index; + if (block_index != -1) { +assert(block_index < num_blks); + +if (var->data.binding == blks[block_index].Binding) { + if (!uniform) + uniform = >data->UniformStorage[i]; + update_uniform_storage(>data->UniformStorage[i], + stage); +} + } + } + + return uniform; + } + + /* Beyond blocks, there are still some corner cases of uniforms without +* location (ie: atomic counters) that would have a initial location equal +* to -1. We just return on that case. Those uniforms will be handled +* later. */ - if (location == -1) + if (var->data.location == -1) return NULL; - for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) - if (prog->data->UniformStorage[i].remap_location == location) + for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) { + if (prog->data->UniformStorage[i].remap_location == var->data.location) { + update_uniform_storage(>data->UniformStorage[i], stage); + return >data->UniformStorage[i]; + } + } return NULL; } @@ -504,9 +563,8 @@ gl_nir_link_uniforms(struct gl_context *ctx, * other stage. If so, validate they are compatible and update * the active stage mask. */ - uniform = find_previous_uniform_storage(prog, var->data.location); + uniform = find_and_update_previous_uniform_storage(prog, var, shader_type); if (uniform) { -uniform->active_shader_mask |= 1 << shader_type; var->data.location = uniform - prog->data->UniformStorage; continue; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 12/28] glsl_types/nir: add explicit_array_stride plus nir wrapper helpers
From ARB_gl_spirv: "Mapping of layouts std140/std430 -> explicit *Offset*, *ArrayStride*, and *MatrixStride* Decoration on struct members" That means that we would not have available any kind of layout info, and we should use explicit array strides. This commit adds explicit_array_stride. The default value is -1 meaning that it is not set (as with offset). That should be the default value for GLSL. In general, the default constructor is ok. We just need to be careful with some array lowerings, as it should try to get the explicit array stride when creating new types. Note that this means that for the ARB_gl_spirv case std430_array_stride, std140_size etc are meaningless (unless you guess the layout, something that you shouldn't). --- src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +- src/compiler/glsl_types.cpp| 28 +- src/compiler/glsl_types.h | 13 +++--- src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 ++- src/compiler/nir/nir_split_per_member_structs.c| 3 ++- src/compiler/nir/nir_split_vars.c | 3 ++- src/compiler/nir_types.cpp | 20 +--- src/compiler/nir_types.h | 10 +++- src/compiler/spirv/vtn_variables.c | 3 ++- 9 files changed, 62 insertions(+), 23 deletions(-) diff --git a/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c b/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c index 9ff5708f503..9716ac4562a 100644 --- a/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c +++ b/src/compiler/glsl/gl_nir_lower_samplers_as_deref.c @@ -99,7 +99,7 @@ remove_struct_derefs_prep(nir_deref_instr **p, char **name, remove_struct_derefs_prep([1], name, location, type); - *type = glsl_get_array_instance(*type, length); + *type = glsl_get_array_instance(*type, length, glsl_get_explicit_array_stride(cur->type)); break; } diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index ed3bb3a9889..1326b21913b 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -44,7 +44,7 @@ glsl_type::glsl_type(GLenum gl_type, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(vector_elements), matrix_columns(matrix_columns), - length(0) + length(0), explicit_array_stride(-1) { /* Values of these types must fit in the two bits of * glsl_type::sampled_type. @@ -77,7 +77,7 @@ glsl_type::glsl_type(GLenum gl_type, glsl_base_type base_type, base_type(base_type), sampled_type(type), sampler_dimensionality(dim), sampler_shadow(shadow), sampler_array(array), interface_packing(0), - interface_row_major(0), length(0) + interface_row_major(0), length(0), explicit_array_stride(-1) { this->mem_ctx = ralloc_context(NULL); assert(this->mem_ctx != NULL); @@ -97,7 +97,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(num_fields) + length(num_fields), explicit_array_stride(-1) { unsigned int i; @@ -127,7 +127,7 @@ glsl_type::glsl_type(const glsl_struct_field *fields, unsigned num_fields, interface_packing((unsigned) packing), interface_row_major((unsigned) row_major), vector_elements(0), matrix_columns(0), - length(num_fields) + length(num_fields), explicit_array_stride(-1) { unsigned int i; @@ -152,7 +152,7 @@ glsl_type::glsl_type(const glsl_type *return_type, sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(num_params) + length(num_params), explicit_array_stride(-1) { unsigned int i; @@ -181,7 +181,7 @@ glsl_type::glsl_type(const char *subroutine_name) : sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(1), matrix_columns(1), - length(0) + length(0), explicit_array_stride(-1) { this->mem_ctx = ralloc_context(NULL); assert(this->mem_ctx != NULL); @@ -434,12 +434,12 @@ _mesa_glsl_release_types(void) } -glsl_type::glsl_type(const glsl_type *array, unsigned length) : +glsl_type::glsl_type(const glsl_type *array, unsigned length, int explicit_array_stride) : base_type(GLSL_TYPE_ARRAY), sampled_type(GLSL_TYPE_VOID), sampler_dimensionality(0), sampler_shadow(0), sampler_array(0), interface_packing(0), interface_row_major(0), vector_elements(0), matrix_columns(0), - length(length), name(NULL) + length(length), name(NULL), explicit_array_stride(explicit_array_stride) { this->fields.array = array; /* Inherit the gl type of the
[Mesa-dev] [PATCH v2 18/28] nir/linker: fill is_shader_storage for uniforms
--- src/compiler/glsl/gl_nir_link_uniforms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index 1a491dc2e5d..00995fb3f76 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -369,6 +369,8 @@ nir_link_uniform(struct gl_context *ctx, if (uniform->hidden) state->num_hidden_uniforms++; + uniform->is_shader_storage = nir_variable_is_in_ssbo(state->current_var); + /* @FIXME: the initialization of the following will be done as we * implement support for their specific features, like SSBO, atomics, * etc. @@ -379,7 +381,6 @@ nir_link_uniform(struct gl_context *ctx, uniform->array_stride = -1; uniform->row_major = false; uniform->builtin = false; - uniform->is_shader_storage = false; uniform->atomic_buffer_index = -1; uniform->top_level_array_size = 0; uniform->top_level_array_stride = 0; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 19/28] nir/linker: use only the array element type for array of ssbo/ubo
For this interfaces, the inner members are added only once as uniforms or resources, in opposite to other cases, like a uniform array of structs. For those guessing why a issue (16) from ARB_program_interface_query was used, instead of a quote of the core spec: The core spec is not really clear about how members of arrays of blocks should be enumerated. On GLSL this was also problematic, specially when we were trying to pass the 4.5 CTS tests. See commit "glsl: Fix program interface queries relating to interface blocks" (4c4d9e4f032d5753034361ee70aa88d16d3a04b4), as a reference. That one also needed to rely on issue (16) to justify the change, pointing that the core spec needs to be clarified. --- As mentioned on the commit message, I needed to quote a issue of a specific extension spec, instead of the core spec. Quoting from the commit "glsl: Fix program interface queries relating to interface blocks", mentioned on the commit message: " There are two important things to note. Those bullet points say "an active interface block", while the others say "variable" or "active shader storage block member". They also don't mention applying the rules recursively (unlike the other bullets). Both suggest that these rules apply to blocks themselves, not members of blocks. In fact, for GL_UNIFORM_BLOCK queries, we do have "block[0]", "block[1]", ... resource list entries - so those rules are real, and actually used. So if they don't apply to block members, then how should members be named? Unfortunately, I don't see any rules outside of issue 16 - where the rationale is very unclear. I hope to clarify the spec in the future." That "clarify the spec in the future" didn't happen. Rules for member of arrays of blocks are not clear with the core spec. This/next week I plan to create a spec issue in order to try to clarify this, even if the solution is just copy what issue 16 says on the core spec. At that point I could send a new commit in order to replace the spec quote. src/compiler/glsl/gl_nir_link_uniforms.c | 44 ++-- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index 00995fb3f76..ac445c8560a 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -498,11 +498,51 @@ gl_nir_link_uniforms(struct gl_context *ctx, state.current_var = var; + /* + * From ARB_program_interface spec, issue (16): + * + * "RESOLVED: We will follow the default rule for enumerating block + * members in the OpenGL API, which is: + * + * * If a variable is a member of an interface block without an + *instance name, it is enumerated using just the variable name. + * + * * If a variable is a member of an interface block with an + *instance name, it is enumerated as "BlockName.Member", where + *"BlockName" is the name of the interface block (not the + *instance name) and "Member" is the name of the variable. + * + * For example, in the following code: + * + * uniform Block1 { + * int member1; + * }; + * uniform Block2 { + * int member2; + * } instance2; + * uniform Block3 { + * int member3; + * } instance3[2]; // uses two separate buffer bindings + * + * the three uniforms (if active) are enumerated as "member1", + * "Block2.member2", and "Block3.member3"." + * + * Note that in the last example, with an array of ubo, only one + * uniform is generated. For that reason, while unrolling the + * uniforms of a ubo, or the variables of a ssbo, we need to treat + * arrays of instance as a single block. + */ + const struct glsl_type *type = var->type; + if (nir_variable_is_in_block(var) && + glsl_type_is_array(type)) { +type = glsl_without_array(type); + } + struct type_tree_entry *type_tree = -build_type_tree_for_type(var->type); +build_type_tree_for_type(type); state.current_type = type_tree; - int res = nir_link_uniform(ctx, prog, sh->Program, shader_type, var->type, + int res = nir_link_uniform(ctx, prog, sh->Program, shader_type, type, location, ); free_type_tree(type_tree); -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 11/28] spirv/nir: fill glsl_struct_field explicit_matrix_stride
--- src/compiler/spirv/spirv_to_nir.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 101e2b0bf02..02de2f640c1 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -798,6 +798,12 @@ struct_member_matrix_stride_cb(struct vtn_builder *b, vtn_assert(mat_type->array_element->stride > 0); mat_type->stride = dec->literals[0]; } + + /* For the glsl_type we use the stride defined at SPIR-V, as anyone (ie: +* ARB_gl_spirv linker) that wants to use it would be also using the matrix +* layout. +*/ + ctx->fields[member].explicit_matrix_stride = dec->literals[0]; } static void -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 15/28] nir/linker/i965: Lower vulkan_resource_index during linking
From: Neil Roberts When linking a program using ARB_gl_spirv it now lowers the vulkan_resource_index intrinsic as an extra pass on the nir shader. Unlike Vulkan this can be done without waiting for the extra state from the pipeline layout. It also adds the call to this lowering on the i965 driver, to avoid a new two-liner patch. --- src/compiler/Makefile.sources | 1 + src/compiler/glsl/gl_nir.h | 4 + .../glsl/gl_nir_lower_vulkan_resource_index.c | 120 + src/compiler/glsl/meson.build | 1 + src/mesa/drivers/dri/i965/brw_link.cpp | 2 + 5 files changed, 128 insertions(+) create mode 100644 src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index d3b06564832..96d00bf95b9 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -28,6 +28,7 @@ LIBGLSL_FILES = \ glsl/gl_nir_lower_atomics.c \ glsl/gl_nir_lower_samplers.c \ glsl/gl_nir_lower_samplers_as_deref.c \ + glsl/gl_nir_lower_vulkan_resource_index.c \ glsl/gl_nir_link_atomics.c \ glsl/gl_nir_link_uniform_initializers.c \ glsl/gl_nir_link_uniforms.c \ diff --git a/src/compiler/glsl/gl_nir.h b/src/compiler/glsl/gl_nir.h index 59d5f65e659..80f56039952 100644 --- a/src/compiler/glsl/gl_nir.h +++ b/src/compiler/glsl/gl_nir.h @@ -30,6 +30,7 @@ extern "C" { struct nir_shader; struct gl_shader_program; +struct gl_linked_shader; bool gl_nir_lower_atomics(nir_shader *shader, const struct gl_shader_program *shader_program, @@ -40,6 +41,9 @@ bool gl_nir_lower_samplers(nir_shader *shader, bool gl_nir_lower_samplers_as_deref(nir_shader *shader, const struct gl_shader_program *shader_program); +bool gl_nir_lower_vulkan_resource_index(nir_shader *shader, +struct gl_linked_shader *linked_shader); + #ifdef __cplusplus } #endif diff --git a/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c new file mode 100644 index 000..92ee3dd707a --- /dev/null +++ b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c @@ -0,0 +1,120 @@ +/* + * 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. + * + * Authors: + *Neil Roberts (nrobe...@igalia.com) + * + */ + +#include "nir.h" +#include "gl_nir.h" +#include "nir_builder.h" +#include "main/mtypes.h" + +/* + * This pass lowers the vulkan_resource_index intrinsic to a surface index. It + * is intended to be used with GL_ARB_gl_spirv. Unlike Vulkan, in that case it + * is not necessary to wait for the complete pipeline state to lower it. + */ + +static unsigned +find_block_by_binding(struct gl_linked_shader *linked_shader, + unsigned binding) +{ + unsigned num_blocks = linked_shader->Program->info.num_ubos; + struct gl_uniform_block **blocks = linked_shader->Program->sh.UniformBlocks; + + for (unsigned i = 0; i < num_blocks; i++) { + if (blocks[i]->Binding == binding) + return i; + } + + unreachable("No block found with the given binding"); +} + +static bool +convert_block(nir_block *block, + struct gl_linked_shader *linked_shader, + nir_builder *b) +{ + bool progress = false; + + nir_foreach_instr_safe(instr, block) { + if (instr->type != nir_instr_type_intrinsic) + continue; + + nir_intrinsic_instr *res_index = nir_instr_as_intrinsic(instr); + + if (res_index->intrinsic != nir_intrinsic_vulkan_resource_index) + continue; + + b->cursor = nir_after_instr(instr); + + /* The descriptor set should always be zero for GL */ + assert(nir_intrinsic_desc_set(res_index) == 0); +
[Mesa-dev] [PATCH v2 14/28] nir: add is_in_ubo/ssbo/block helpers
Equivalent to the already existing ir_variable is_in_buffer_block and is_in_shader_storage_block, adding the uniform buffer object one. I'm using the short forms (ssbo, ubo) to avoid having method names too long. --- src/compiler/nir/nir.h | 22 ++ 1 file changed, 22 insertions(+) diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index e0df95c391c..49d1e7997e5 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -3071,6 +3071,28 @@ uint64_t nir_get_single_slot_attribs_mask(uint64_t attribs, uint64_t dual_slot); nir_intrinsic_op nir_intrinsic_from_system_value(gl_system_value val); gl_system_value nir_system_value_from_intrinsic(nir_intrinsic_op intrin); + +static inline bool +nir_variable_is_in_ubo(const nir_variable *var) +{ + return (var->data.mode == nir_var_uniform && + var->interface_type != NULL); +} + +static inline bool +nir_variable_is_in_ssbo(const nir_variable *var) +{ + return (var->data.mode == nir_var_shader_storage && + var->interface_type != NULL); +} + +static inline bool +nir_variable_is_in_block(const nir_variable *var) +{ + return nir_variable_is_in_ubo(var) || nir_variable_is_in_ssbo(var); +} + + #ifdef __cplusplus } /* extern "C" */ #endif -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 20/28] nir/linker: fill up uniform_storage with explicit data
Specifically, offset, array_stride, matrix_stride and row_major. On GLSL, most of that info is computed, but on ARB_gl_spirv they are explicit, and for Mesa, included on the glsl_type. From ARB_gl_spirv spec: "Mapping of layouts std140/std430 -> explicit *Offset*, *ArrayStride*, and *MatrixStride* Decoration on struct members"" "7.6.2.spv SPIR-V Uniform Offsets and Strides The SPIR-V decorations *GLSLShared* or *GLSLPacked* must not be used. A variable in the *Uniform* Storage Class decorated as a *Block* must be explicitly laid out using the *Offset*, *ArrayStride*, and *MatrixStride* decorations" For offset, matrix_stride and row_major we needed to include the parent and index_in_parent while processing the type, as matrix_stride/row_major are maintained as fields of the parent type, not on the type itself. --- src/compiler/glsl/gl_nir_link_uniforms.c | 31 ++- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c b/src/compiler/glsl/gl_nir_link_uniforms.c index ac445c8560a..448f8277c16 100644 --- a/src/compiler/glsl/gl_nir_link_uniforms.c +++ b/src/compiler/glsl/gl_nir_link_uniforms.c @@ -282,6 +282,8 @@ nir_link_uniform(struct gl_context *ctx, struct gl_program *stage_program, gl_shader_stage stage, const struct glsl_type *type, + const struct glsl_type *parent_type, + unsigned index_in_parent, int location, struct nir_link_uniforms_state *state) { @@ -309,7 +311,7 @@ nir_link_uniform(struct gl_context *ctx, field_type = glsl_get_array_element(type); int entries = nir_link_uniform(ctx, prog, stage_program, stage, -field_type, location, +field_type, type, i, location, state); if (entries == -1) return -1; @@ -352,9 +354,11 @@ nir_link_uniform(struct gl_context *ctx, if (glsl_type_is_array(type)) { uniform->type = type_no_array; uniform->array_elements = glsl_get_length(type); + uniform->array_stride = glsl_get_explicit_array_stride(type); } else { uniform->type = type; uniform->array_elements = 0; + uniform->array_stride = 0; } uniform->active_shader_mask |= 1 << stage; @@ -371,15 +375,31 @@ nir_link_uniform(struct gl_context *ctx, uniform->is_shader_storage = nir_variable_is_in_ssbo(state->current_var); + if (nir_variable_is_in_block(state->current_var) && + glsl_type_is_matrix(type)) { + assert(parent_type); + + uniform->matrix_stride = +glsl_get_struct_field_explicit_matrix_stride(parent_type, index_in_parent); + + uniform->row_major = +glsl_get_struct_field_matrix_layout(parent_type, index_in_parent) == +GLSL_MATRIX_LAYOUT_ROW_MAJOR; + } else { + uniform->matrix_stride = 0; + uniform->row_major = false; + } + + if (parent_type) + uniform->offset = glsl_get_struct_field_offset(parent_type, index_in_parent); + else + uniform->offset = 0; + /* @FIXME: the initialization of the following will be done as we * implement support for their specific features, like SSBO, atomics, * etc. */ uniform->block_index = -1; - uniform->offset = -1; - uniform->matrix_stride = -1; - uniform->array_stride = -1; - uniform->row_major = false; uniform->builtin = false; uniform->atomic_buffer_index = -1; uniform->top_level_array_size = 0; @@ -543,6 +563,7 @@ gl_nir_link_uniforms(struct gl_context *ctx, state.current_type = type_tree; int res = nir_link_uniform(ctx, prog, sh->Program, shader_type, type, +NULL, 0, location, ); free_type_tree(type_tree); -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 13/28] spirv/nir: fill glsl_type array stride
We need all the info when asking for the type, so we needed to call type_decoration_cb earlier, in order to get the ArrayStride. It is somewhat ugly to do this only for Array types, but we can't do it before the switch as type_decoration_cb have some asserts to ensure that the type and the decoration are compatible. One alternative would be keep the call to type_decoration_cb at the end, but create the glsl type for Arrays at the end, after calling it. Again we are treating Arrays in a different way. A full alternative to treat all types in the same way would be have a first switch(opcode) that would fill the base_type, call type_decoration_cb, and then a new switch(opcode) that would fill extra data and create the glsl_type. That looks like an overkill though. --- src/compiler/spirv/spirv_to_nir.c | 15 --- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 02de2f640c1..8d609f1ddb5 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -1125,9 +1125,14 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode, } val->type->base_type = vtn_base_type_array; - val->type->type = glsl_array_type(array_element->type, val->type->length); + /* We need to call type_decoration_cb earlier, in order to get the + * proper value of ArrayStride + */ + vtn_foreach_decoration(b, val, type_decoration_cb, NULL); + + val->type->type = glsl_full_array_type(array_element->type, val->type->length, + val->type->stride); val->type->array_element = array_element; - val->type->stride = 0; break; } @@ -1306,7 +1311,11 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode, vtn_fail("Unhandled opcode"); } - vtn_foreach_decoration(b, val, type_decoration_cb, NULL); + /* For Arrays we already called foreach_decoration */ + if (opcode != SpvOpTypeRuntimeArray && opcode != SpvOpTypeArray) { + vtn_foreach_decoration(b, val, type_decoration_cb, NULL); + } + } static nir_constant * -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 17/28] nir/linker: add gl_nir_link_uniform_blocks.c
Adding the ability to link uniform blocks and shader storage blocks using NIR, intended for ARB_gl_spirv support. Among other things, this linking needs to take into account that everything should work without names, as they could be not present, while the GLSL IR uniform block linking was wrote with the names on its core. The other major difference compared with the GLSL IR linker is that we don't deal with layouts. There are no references to std140, std430, etc. Layouts are expressed through explicit offset, array stride and matrix stride. That simplifies how the buffer size are computed. But also means that we can't use the existing methods at glsl_types, so it is mostly computed here. This code only exposes the method gl_nir_link_uniform_blocks on gl_nir_linker.h It is worth to note that this linking do a iteration over the glsl_types, similarly to what the uniform linking do. A possible future improvement would be refactor both cases to try to share more code that it sharing right now. On GLSL IR there are a class visitor, specialized on each case, for that sharing. As adding a class visitor on C would more complicated, for now we are just iterating on both. Signed-off-by: Alejandro Piñeiro Signed-off-by: Neil Roberts --- src/compiler/Makefile.sources | 1 + src/compiler/glsl/gl_nir_link_uniform_blocks.c | 713 + src/compiler/glsl/gl_nir_linker.h | 3 + src/compiler/glsl/meson.build | 1 + 4 files changed, 718 insertions(+) create mode 100644 src/compiler/glsl/gl_nir_link_uniform_blocks.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index 96d00bf95b9..3a9f169ae62 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -30,6 +30,7 @@ LIBGLSL_FILES = \ glsl/gl_nir_lower_samplers_as_deref.c \ glsl/gl_nir_lower_vulkan_resource_index.c \ glsl/gl_nir_link_atomics.c \ + glsl/gl_nir_link_uniform_blocks.c \ glsl/gl_nir_link_uniform_initializers.c \ glsl/gl_nir_link_uniforms.c \ glsl/gl_nir_link_xfb.c \ diff --git a/src/compiler/glsl/gl_nir_link_uniform_blocks.c b/src/compiler/glsl/gl_nir_link_uniform_blocks.c new file mode 100644 index 000..8dd0bb6f71f --- /dev/null +++ b/src/compiler/glsl/gl_nir_link_uniform_blocks.c @@ -0,0 +1,713 @@ +/* + * Copyright © 2017 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.h" +#include "gl_nir_linker.h" +#include "ir_uniform.h" /* for gl_uniform_storage */ +#include "linker_util.h" +#include "main/shaderobj.h" /* _mesa_delete_linked_shader */ +#include "main/mtypes.h" + +/* Summary: This file contains code to do a nir-based linking for uniform + * blocks. This includes ubos and ssbos. + * + * More details: + * + * 1. Note that it is tailored to ARB_gl_spirv needs. Uniform block name, + * fields names, and other names are considered optional debug infor so could + * not be present. So the linking should work without it, and it is optional + * to not handle them at all. From ARB_gl_spirv: + * + *"19. How should the program interface query operations behave for program + * objects created from SPIR-V shaders? + * + * DISCUSSION: we previously said we didn't need reflection to work for + * SPIR-V shaders (at least for the first version), however we are left + * with specifying how it should "not work". The primary issue is that + * SPIR-V binaries are not required to have names associated with + * variables. They can be associated in debug information, but there is no + * requirement for that to be present, and it should not be relied upon. + * + * Options: + * + * + * + *C) Allow as much as possible to work "natur
[Mesa-dev] [PATCH v2 16/28] nir/linker: handle non-ubo uses of vulkan_resource_index
From: Neil Roberts In order to replicate the behaviour of lower_ubo_reference_visitor, the lowering code should search the list of blocks in ShaderStorageBlocks for the matching binding whenever a non-ubo usage of the resource index is encountered. The intended usage of the vulkan_resource_index is determined by searching for an intrinsic which uses the result. Unfortunately some other lower passes can add instructions to perform arithmetic on the result so the search needs to be performed recursively on the result of those. Signed-off-by: Neil Roberts Signed-off-by: Alejandro Piñeiro --- .../glsl/gl_nir_lower_vulkan_resource_index.c | 55 +++--- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c index 92ee3dd707a..561d2a03de2 100644 --- a/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c +++ b/src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c @@ -37,12 +37,10 @@ */ static unsigned -find_block_by_binding(struct gl_linked_shader *linked_shader, +find_block_by_binding(unsigned num_blocks, + struct gl_uniform_block **blocks, unsigned binding) { - unsigned num_blocks = linked_shader->Program->info.num_ubos; - struct gl_uniform_block **blocks = linked_shader->Program->sh.UniformBlocks; - for (unsigned i = 0; i < num_blocks; i++) { if (blocks[i]->Binding == binding) return i; @@ -51,6 +49,35 @@ find_block_by_binding(struct gl_linked_shader *linked_shader, unreachable("No block found with the given binding"); } +static bool +find_intrinsic_usage(nir_ssa_def *def, + bool *is_ubo_usage) +{ + nir_foreach_use_safe(use_src, def) { + if (use_src->parent_instr->type == nir_instr_type_alu) { + nir_alu_instr *alu = nir_instr_as_alu(use_src->parent_instr); + + if (find_intrinsic_usage(>dest.dest.ssa, is_ubo_usage)) +return true; + + continue; + } + + if (use_src->parent_instr->type != nir_instr_type_intrinsic) + continue; + + nir_intrinsic_instr *intr = nir_instr_as_intrinsic(use_src->parent_instr); + + if (intr == NULL) + continue; + + *is_ubo_usage = intr->intrinsic == nir_intrinsic_load_ubo; + return true; + } + + return false; +} + static bool convert_block(nir_block *block, struct gl_linked_shader *linked_shader, @@ -67,13 +94,29 @@ convert_block(nir_block *block, if (res_index->intrinsic != nir_intrinsic_vulkan_resource_index) continue; + bool is_ubo_usage; + if (!find_intrinsic_usage(_index->dest.ssa, _ubo_usage)) + continue; + b->cursor = nir_after_instr(instr); /* The descriptor set should always be zero for GL */ assert(nir_intrinsic_desc_set(res_index) == 0); - unsigned binding = nir_intrinsic_binding(res_index); - unsigned block = find_block_by_binding(linked_shader, binding); + + unsigned num_blocks; + struct gl_uniform_block **blocks; + + if (is_ubo_usage) { + num_blocks = linked_shader->Program->info.num_ubos; + blocks = linked_shader->Program->sh.UniformBlocks; + } else { + num_blocks = linked_shader->Program->info.num_ssbos; + blocks = linked_shader->Program->sh.ShaderStorageBlocks; + } + + unsigned block = find_block_by_binding(num_blocks, blocks, binding); + nir_ssa_def *surface = nir_iadd(b, nir_imm_int(b, block), -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 00/28] ARB_gl_spirv: v2 ubo/ssbo support, plus CTS goodness
Hi, this is the second version of the ubo/ssbo support for ARB_gl_spirv series. Differences compared with v1: * Rebased against today master. * Patch "nir/linker: use only the array element type for array of ssbo/ubo" got a proper spec quote. * Two extra patches are included. They are not strictly related to ubo/ssbo. Just when I sent v1, I checked how many patches from our development branch were required to pass all the ARB_gl_spirv tests. And they were just two. So although initially the plan was sending them as part of a different series, we decided to send it now, basically because "passing all the CTS tests" is a nice checkpoint, and it would be nice to get it on master. Having said so, note that "passing all the CTS tests" is not "extension is production ready", so we would keep sending series/patches before propose to enable it. And something that was not mentioned on v1 cover-letter, but later on the review thread: this series doesn't include the support for array of arrays of ubo/ssbo. That will be sent later on a different series. The v1 was sent here: https://lists.freedesktop.org/archives/mesa-dev/2018-September/205278.html This series can be found on this branch: https://github.com/Igalia/piglit/tree/arb_gl_spirv-series5-ubo-ssbo-v2 And can be tested with this piglit series: https://github.com/Igalia/piglit/tree/arb_gl_spirv-series5-ubo-ssbo-v2 And as mentioned, with this series would pass the CTS ARB_gl_spirv tests on i965. Alejandro Piñeiro (23): spirv/nir: translate uniform blocks spirv/nir: translate ssbo spirv/nir: setting interface type for ubos/ssbos spirv/nir: fill up nir variable info for ubos and ssbo spirv/nir: include SPIR-V explicit offset on the glsl struct type spirv/nir: include row major coming from SPIR-V on the glsl type spirv/nir: don't set interface_type if it is not a struct nir/types: add three new wrapper helpers glsl_types/nir: add matrix_stride plus nir wrapper helpers spirv/nir: fill glsl_struct_field explicit_matrix_stride glsl_types/nir: add explicit_array_stride plus nir wrapper helpers spirv/nir: fill glsl_type array stride nir: add is_in_ubo/ssbo/block helpers nir/linker: add gl_nir_link_uniform_blocks.c nir/linker: fill is_shader_storage for uniforms nir/linker: use only the array element type for array of ssbo/ubo nir/linker: fill up uniform_storage with explicit data nir/linker: update already processed uniforms search for UBOs/SSBOs nir/linker: add program ubo/ssbo at the resource list i965: use GLboolean for all brw_link_shader returns i965: call to gl_nir_link_uniform_blocks mesa: add NULL name check for NUM_ACTIVE_VARIABLES query mesa: add NULL name check for several length queries Antia Puentes (2): nir/linker: Set the uniform's block_index nir/linker: Add inputs/outputs to the program resource list Neil Roberts (3): spirv/nir: Handle location decorations on block interface members nir/linker/i965: Lower vulkan_resource_index during linking nir/linker: handle non-ubo uses of vulkan_resource_index src/compiler/Makefile.sources | 2 + src/compiler/glsl/gl_nir.h | 4 + src/compiler/glsl/gl_nir_link_uniform_blocks.c | 713 + src/compiler/glsl/gl_nir_link_uniforms.c | 178 - src/compiler/glsl/gl_nir_linker.c | 93 +++ src/compiler/glsl/gl_nir_linker.h | 3 + src/compiler/glsl/gl_nir_lower_samplers_as_deref.c | 2 +- .../glsl/gl_nir_lower_vulkan_resource_index.c | 163 + src/compiler/glsl/meson.build | 2 + src/compiler/glsl_types.cpp| 31 +- src/compiler/glsl_types.h | 23 +- src/compiler/nir/nir.h | 22 + src/compiler/nir/nir_lower_io_arrays_to_elements.c | 3 +- src/compiler/nir/nir_split_per_member_structs.c| 3 +- src/compiler/nir/nir_split_vars.c | 3 +- src/compiler/nir_types.cpp | 47 +- src/compiler/nir_types.h | 20 +- src/compiler/spirv/spirv_to_nir.c | 24 +- src/compiler/spirv/vtn_private.h | 6 + src/compiler/spirv/vtn_variables.c | 90 ++- src/mesa/drivers/dri/i965/brw_link.cpp | 12 +- src/mesa/main/shader_query.cpp | 42 +- src/mesa/main/shaderapi.c | 26 +- 23 files changed, 1430 insertions(+), 82 deletions(-) create mode 100644 src/compiler/glsl/gl_nir_link_uniform_blocks.c create mode 100644 src/compiler/glsl/gl_nir_lower_vulkan_resource_index.c -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 02/28] spirv/nir: translate ssbo
They are supported by SPIR-V for OpenGL. OpenGL codepath expect nir to include the ssbo as nir variables. --- src/compiler/spirv/vtn_variables.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c index ba1b8816038..53359c41005 100644 --- a/src/compiler/spirv/vtn_variables.c +++ b/src/compiler/spirv/vtn_variables.c @@ -1482,7 +1482,7 @@ vtn_storage_class_to_mode(struct vtn_builder *b, nir_mode = nir_var_uniform; } else if (interface_type->buffer_block) { mode = vtn_variable_mode_ssbo; - nir_mode = 0; + nir_mode = nir_var_shader_storage; } else { /* Default-block uniforms, coming from gl_spirv */ mode = vtn_variable_mode_uniform; @@ -1678,6 +1678,7 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, case vtn_variable_mode_global: case vtn_variable_mode_uniform: case vtn_variable_mode_ubo: + case vtn_variable_mode_ssbo: /* For these, we create the variable normally */ var->var = rzalloc(b->shader, nir_variable); var->var->name = ralloc_strdup(var->var, val->name); @@ -1782,7 +1783,6 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val, break; } - case vtn_variable_mode_ssbo: case vtn_variable_mode_push_constant: /* These don't need actual variables. */ break; -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2 10/28] glsl_types/nir: add matrix_stride plus nir wrapper helpers
From ARB_gl_spirv spec: "7.6.2.spv SPIR-V Uniform Offsets and Strides The SPIR-V decorations *GLSLShared* or *GLSLPacked* must not be used. A variable in the *Uniform* Storage Class decorated as a *Block* must be explicitly laid out using the *Offset*, *ArrayStride*, and *MatrixStride* decorations. If the variable is decorated as a *BufferBlock*, its offsets and strides must not contradict std430 alignment and minimum offset requirements. Otherwise, its offsets and strides must not contradict std140 alignment and minimum offset requirements. From that paragraph, the first conclusion is that we can rely on the content of the SPIR-V in order to compute the buffer sizes, as they are mandatory. That would make the buffer size computation easier. The second conclusion, from the last sentence, is that *we need* to do that. As if just needs to not contradict alignments and minimum offsets, providing a matrix stride of 16 when 8 is enough would be valid. This explicit matrix_stride is assumed to only be used on ARB_gl_spirv. On GLSL there is no way to set it, and it is internally handled and computed. --- src/compiler/glsl_types.cpp | 3 +++ src/compiler/glsl_types.h | 10 -- src/compiler/nir_types.cpp | 6 ++ src/compiler/nir_types.h| 2 ++ 4 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/compiler/glsl_types.cpp b/src/compiler/glsl_types.cpp index ca5368aa53f..ed3bb3a9889 100644 --- a/src/compiler/glsl_types.cpp +++ b/src/compiler/glsl_types.cpp @@ -961,6 +961,9 @@ glsl_type::record_compare(const glsl_type *b, bool match_locations) const if (this->fields.structure[i].xfb_stride != b->fields.structure[i].xfb_stride) return false; + if (this->fields.structure[i].explicit_matrix_stride + != b->fields.structure[i].explicit_matrix_stride) + return false; } return true; diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h index d32b580acc1..9e8332e6cbf 100644 --- a/src/compiler/glsl_types.h +++ b/src/compiler/glsl_types.h @@ -1007,6 +1007,12 @@ struct glsl_struct_field { */ unsigned matrix_layout:2; + /** +* Explicit matrix stride. For ARB_gl_spirv, it is mandatory to set it +* explicitly. -1 otherwise. +*/ + int explicit_matrix_stride; + /** * For interface blocks, 1 if this variable is a per-patch input or output * (as in ir_variable::patch). 0 otherwise. @@ -1045,7 +1051,7 @@ struct glsl_struct_field { glsl_struct_field(const struct glsl_type *_type, const char *_name) : type(_type), name(_name), location(-1), offset(0), xfb_buffer(0), xfb_stride(0), interpolation(0), centroid(0), -sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), patch(0), +sample(0), matrix_layout(GLSL_MATRIX_LAYOUT_INHERITED), explicit_matrix_stride(-1), patch(0), precision(GLSL_PRECISION_NONE), memory_read_only(0), memory_write_only(0), memory_coherent(0), memory_volatile(0), memory_restrict(0), image_format(0), explicit_xfb_buffer(0), @@ -1057,7 +1063,7 @@ struct glsl_struct_field { glsl_struct_field() : type(NULL), name(NULL), location(0), offset(0), xfb_buffer(0), xfb_stride(0), interpolation(0), centroid(0), -sample(0), matrix_layout(0), patch(0), +sample(0), matrix_layout(0), explicit_matrix_stride(-1), patch(0), precision(0), memory_read_only(0), memory_write_only(0), memory_coherent(0), memory_volatile(0), memory_restrict(0), image_format(0), explicit_xfb_buffer(0), diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp index 2a1ae42a9bb..b2a5da1dc6c 100644 --- a/src/compiler/nir_types.cpp +++ b/src/compiler/nir_types.cpp @@ -86,6 +86,12 @@ glsl_get_struct_field_matrix_layout(const struct glsl_type *type, return type->fields.structure[index].matrix_layout; } +const int +glsl_get_struct_field_explicit_matrix_stride(const struct glsl_type *type, + unsigned index) +{ + return type->fields.structure[index].explicit_matrix_stride; +} const glsl_type * glsl_get_function_return_type(const glsl_type *type) diff --git a/src/compiler/nir_types.h b/src/compiler/nir_types.h index 69de44c3423..d3c00ca5e1a 100644 --- a/src/compiler/nir_types.h +++ b/src/compiler/nir_types.h @@ -51,6 +51,8 @@ const int glsl_get_struct_field_offset(const struct glsl_type *type, const unsigned glsl_get_struct_field_matrix_layout(const struct glsl_type *type, unsigned index); +const int glsl_get_struct_field_explicit_matrix_stride(const struct glsl_type *type, + unsigned index); const struct glsl_type *glsl_get_array_element(const struct glsl_type *type); const struct glsl_type *glsl_without_array(const struct glsl_type *type); const struct glsl_type
[Mesa-dev] [PATCH v2 06/28] spirv/nir: include row major coming from SPIR-V on the glsl type
--- src/compiler/spirv/spirv_to_nir.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c index 15a3e8cce9a..101e2b0bf02 100644 --- a/src/compiler/spirv/spirv_to_nir.c +++ b/src/compiler/spirv/spirv_to_nir.c @@ -725,6 +725,7 @@ struct_member_decoration_cb(struct vtn_builder *b, break; /* Nothing to do here. Column-major is the default. */ case SpvDecorationRowMajor: mutable_matrix_member(b, ctx->type, member)->row_major = true; + ctx->fields[member].matrix_layout = GLSL_MATRIX_LAYOUT_ROW_MAJOR; break; case SpvDecorationPatch: -- 2.14.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev