[Mesa-dev] [PATCH 1/3] nir: Add a lowering pass for gl_FragColor to glFragData[] writes.
Kenneth Graunke
kenneth at whitecape.org
Fri Dec 29 08:00:58 UTC 2017
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 <kenneth at whitecape.org>
though my review on your vc5 patch is a pretty low quality review :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171229/37922143/attachment-0001.sig>
More information about the mesa-dev
mailing list