On Thursday, December 28, 2017 5:56:18 PM PST Eric Anholt wrote: > For VC5, the shader needs to have the appropriate base type for the > variable in the render target write, and gallium's > FS_COLOR0_WRITES_ALL_CBUFS (used for glClearBufferiv) doesn't give you > that information. This pass lets the backend decide what types to explode > the gl_FragColor write out to. > > This would also be a prerequisite of moving some of VC5's render target > format packing into NIR as well.
This type confusion seems a bit unfortunate. Both gl_FragColor and gl_FragData[i] are always float vec4s. But, we've used (abused?) it in the past for integer clear/blit paths, since it's the only way to splat out to all render targets. I suppose this must be an internally generated shader? Or, do you also need this for people writing to gl_FragColor but binding integer render targets? (If so, what about people declaring 'out vec4 foo' but binding an integer render target?) > --- > src/compiler/Makefile.sources | 1 + > src/compiler/nir/meson.build | 1 + > src/compiler/nir/nir.h | 3 + > src/compiler/nir/nir_lower_gl_fragcolor.c | 143 > ++++++++++++++++++++++++++++++ > 4 files changed, 148 insertions(+) > create mode 100644 src/compiler/nir/nir_lower_gl_fragcolor.c > > diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources > index d3f746f5f948..4afaa1a2146a 100644 > --- a/src/compiler/Makefile.sources > +++ b/src/compiler/Makefile.sources > @@ -220,6 +220,7 @@ NIR_FILES = \ > nir/nir_lower_constant_initializers.c \ > nir/nir_lower_double_ops.c \ > nir/nir_lower_drawpixels.c \ > + nir/nir_lower_gl_fragcolor.c \ > nir/nir_lower_global_vars_to_local.c \ > nir/nir_lower_gs_intrinsics.c \ > nir/nir_lower_load_const_to_scalar.c \ > diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build > index 5dd21e6652f0..9e11279118f6 100644 > --- a/src/compiler/nir/meson.build > +++ b/src/compiler/nir/meson.build > @@ -114,6 +114,7 @@ files_libnir = files( > 'nir_lower_constant_initializers.c', > 'nir_lower_double_ops.c', > 'nir_lower_drawpixels.c', > + 'nir_lower_gl_fragcolor.c', > 'nir_lower_global_vars_to_local.c', > 'nir_lower_gs_intrinsics.c', > 'nir_lower_load_const_to_scalar.c', > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > index 440c3fe9974c..17bb8fc8de4c 100644 > --- a/src/compiler/nir/nir.h > +++ b/src/compiler/nir/nir.h > @@ -2680,6 +2680,9 @@ bool nir_lower_atomics(nir_shader *shader, > bool nir_lower_atomics_to_ssbo(nir_shader *shader, unsigned ssbo_offset); > bool nir_lower_uniforms_to_ubo(nir_shader *shader); > bool nir_lower_to_source_mods(nir_shader *shader); > +bool nir_lower_gl_fragcolor(nir_shader *shader, > + uint32_t rt_mask, > + const struct glsl_type **types); > > bool nir_lower_gs_intrinsics(nir_shader *shader); > > diff --git a/src/compiler/nir/nir_lower_gl_fragcolor.c > b/src/compiler/nir/nir_lower_gl_fragcolor.c > new file mode 100644 > index 000000000000..d4b39f00c233 > --- /dev/null > +++ b/src/compiler/nir/nir_lower_gl_fragcolor.c > @@ -0,0 +1,143 @@ > +/* > + * Copyright © 2017 Broadcom > + * > + * 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 "nir_builder.h" > + > +/** > + * Lowers gl_FragColor to a per-render-target store. > + * > + * GLSL's gl_FragColor writes implicitly broadcast their store to every > active > + * render target. This can be used by driver backends to implement > + * gl_FragColor in the same way as other multiple-render-target shaders, and > + * is particularly useful if the driver needs to do other per-render-target > + * lowering in NIR. > + * > + * Run before nir_lower_io. > + */ > + > +typedef struct { Let's avoid typedefs here - this is just a struct containing the state of the pass. We do use typedefs in NIR, but mostly for core concepts. This isn't anything that special or fundamental (and most passes just use a plain struct). > + nir_shader *shader; > + nir_builder b; > + > + nir_variable *var; /* gl_FragColor */ > + > + int num_rt_vars; > + nir_variable *rt_var[32]; /* gl_FragDataN */ > +} lower_gl_fragcolor_state; > + > +static void > +lower_gl_fragcolor(lower_gl_fragcolor_state *state, nir_intrinsic_instr > *intr) > +{ > + nir_builder *b = &state->b; > + > + assert(intr->dest.is_ssa); > + > + b->cursor = nir_before_instr(&intr->instr); > + > + /* Generate a gl_FragDataN write per render target. */ > + nir_ssa_def *color = nir_ssa_for_src(b, intr->src[0], 4); > + for (int i = 0; i < state->num_rt_vars; i++) { > + nir_store_var(b, state->rt_var[i], color, 0xf); > + } > + > + /* Remove the gl_FragColor write. */ > + nir_instr_remove(&intr->instr); > +} > + > +static bool > +lower_gl_fragcolor_block(lower_gl_fragcolor_state *state, nir_block *block) > +{ > + nir_foreach_instr_safe(instr, block) { > + if (instr->type == nir_instr_type_intrinsic) { > + nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr); > + if (intr->intrinsic == nir_intrinsic_store_var) { > + nir_deref_var *dvar = intr->variables[0]; > + nir_variable *var = dvar->var; > + > + if (var == state->var) { > + /* gl_FragColor should not have array/struct derefs: */ > + assert(dvar->deref.child == NULL); > + lower_gl_fragcolor(state, intr); > + } > + } > + } > + } > + > + return true; > +} > + > +bool > +nir_lower_gl_fragcolor(nir_shader *shader, const uint32_t rt_mask, > + const struct glsl_type **types) > +{ > + lower_gl_fragcolor_state state = { > + .shader = shader, > + }; > + > + assert(shader->info.stage == MESA_SHADER_FRAGMENT); > + > + nir_foreach_variable(var, &shader->outputs) { > + if (var->data.location == FRAG_RESULT_COLOR) { > + state.var = var; > + break; > + } > + } > + > + if (!state.var) > + return false; > + > + for (int i = 0; i < ARRAY_SIZE(state.rt_var); i++) { > + if (!(rt_mask & (1 << i))) > + continue; > + > + char name[16]; > + snprintf(name, sizeof(name), "gl_FragData%d", i); > + Earlier, you generated store_vars with a writemask of 0xf, which assumes that these rt_var variables are gvec4's. That's only reasonable if the underlying types are a gvec4. Since you allow the caller to pass in an array of -any- glsl_type, it might be nice to enforce that here: assert(glsl_get_components(types[i]) == 4); That should guard against the caller passing in bogus types with too few components for gl_FragColor lowering, or arrays/structures/craziness. > + nir_variable *rt_var = nir_variable_create(state.shader, > + nir_var_shader_out, > + types[i], > + name); > + rt_var->data.location = FRAG_RESULT_DATA0 + i; > + > + state.rt_var[state.num_rt_vars++] = rt_var; > + } > + > + nir_foreach_function(function, shader) { > + if (function->impl) { > + nir_builder_init(&state.b, function->impl); > + > + nir_foreach_block(block, function->impl) { > + lower_gl_fragcolor_block(&state, block); > + } > + > + nir_metadata_preserve(function->impl, > + nir_metadata_block_index | > + nir_metadata_dominance); > + } > + } > + > + exec_node_remove(&state.var->node); > + > + return true; > +} I might be interested in using this pass in i965 - it wouldn't save us much code, but dropping our backend's gl_FragColor-splatting would be kinda nice regardless... Series is: Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> though my review on your vc5 patch is a pretty low quality review :)
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev