Discussion:
[PATCH 2/3] nir: Add a helper to get the uvec4 type.
Add Reply
Eric Anholt
2017-12-29 01:56:19 UTC
Reply
Permalink
Raw Message
I needed this in the vc5 compiler.
---
src/compiler/nir_types.cpp | 6 ++++++
src/compiler/nir_types.h | 1 +
2 files changed, 7 insertions(+)

diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
index 377de0c9c7bd..cbdd452dc813 100644
--- a/src/compiler/nir_types.cpp
+++ b/src/compiler/nir_types.cpp
@@ -297,6 +297,12 @@ glsl_vec4_type(void)
return glsl_type::vec4_type;
}

+const glsl_type *
+glsl_uvec4_type(void)
+{
+ return glsl_type::uvec4_type;
+}
+
const glsl_type *
glsl_int_type(void)
{
diff --git a/src/compiler/nir_types.h b/src/compiler/nir_types.h
index daff97325093..4397c2406f9a 100644
--- a/src/compiler/nir_types.h
+++ b/src/compiler/nir_types.h
@@ -136,6 +136,7 @@ const struct glsl_type *glsl_double_type(void);
const struct glsl_type *glsl_vec_type(unsigned n);
const struct glsl_type *glsl_dvec_type(unsigned n);
const struct glsl_type *glsl_vec4_type(void);
+const struct glsl_type *glsl_uvec4_type(void);
const struct glsl_type *glsl_int_type(void);
const struct glsl_type *glsl_uint_type(void);
const struct glsl_type *glsl_int64_t_type(void);
--
2.15.0
Eric Anholt
2017-12-29 01:56:20 UTC
Reply
Permalink
Raw Message
This fixes dEQP-GLES3.functional.fbo.color.clear.r16i and friends, by
making sure we do an integer TLB store instead of float.
---
src/broadcom/compiler/nir_to_vir.c | 5 +----
src/broadcom/compiler/v3d_compiler.h | 6 ++++++
src/broadcom/compiler/vir.c | 13 +++++++++++++
src/gallium/drivers/vc5/vc5_program.c | 5 +++++
4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/src/broadcom/compiler/nir_to_vir.c b/src/broadcom/compiler/nir_to_vir.c
index 1cf8865bf0e1..4bd9ae2e9a74 100644
--- a/src/broadcom/compiler/nir_to_vir.c
+++ b/src/broadcom/compiler/nir_to_vir.c
@@ -1493,10 +1493,7 @@ ntq_setup_outputs(struct v3d_compile *c)
if (c->s->info.stage == MESA_SHADER_FRAGMENT) {
switch (var->data.location) {
case FRAG_RESULT_COLOR:
- c->output_color_var[0] = var;
- c->output_color_var[1] = var;
- c->output_color_var[2] = var;
- c->output_color_var[3] = var;
+ unreachable("Should have been lowered");
break;
case FRAG_RESULT_DATA0:
case FRAG_RESULT_DATA1:
diff --git a/src/broadcom/compiler/v3d_compiler.h b/src/broadcom/compiler/v3d_compiler.h
index bbe7a57fa10e..d060af3c4169 100644
--- a/src/broadcom/compiler/v3d_compiler.h
+++ b/src/broadcom/compiler/v3d_compiler.h
@@ -322,6 +322,12 @@ struct v3d_fs_key {
uint8_t swap_color_rb;
/* Mask of which render targets need to be written as 32-bit floats */
uint8_t f32_color_rb;
+
+ /* Mask of which render targets need gl_FragColor output as a vec4. */
+ uint8_t gl_fragcolor_lower_vec4;
+ /* Mask of which render targets need gl_FragColor output as a uvec4. */
+ uint8_t gl_fragcolor_lower_uvec4;
+
uint8_t alpha_test_func;
uint8_t logicop_func;
uint32_t point_sprite_mask;
diff --git a/src/broadcom/compiler/vir.c b/src/broadcom/compiler/vir.c
index 4e78a477bd7d..abcb430c6e3b 100644
--- a/src/broadcom/compiler/vir.c
+++ b/src/broadcom/compiler/vir.c
@@ -750,6 +750,19 @@ uint64_t *v3d_compile_fs(const struct v3d_compiler *compiler,
if (key->base.ucp_enables)
NIR_PASS_V(c->s, nir_lower_clip_fs, key->base.ucp_enables);

+ const struct glsl_type *gl_fragcolor_types[4] = {NULL, NULL, NULL, NULL};
+ for (int i = 0; i < ARRAY_SIZE(gl_fragcolor_types); i++) {
+ if (key->gl_fragcolor_lower_vec4 & (1 << i))
+ gl_fragcolor_types[i] = glsl_vec4_type();
+ else if (key->gl_fragcolor_lower_uvec4 & (1 << i))
+ gl_fragcolor_types[i] = glsl_uvec4_type();
+ }
+
+ NIR_PASS_V(c->s, nir_lower_gl_fragcolor,
+ key->gl_fragcolor_lower_vec4 |
+ key->gl_fragcolor_lower_uvec4,
+ gl_fragcolor_types);
+
/* Note: FS input scalarizing must happen after
* nir_lower_two_sided_color, which only handles a vec4 at a time.
*/
diff --git a/src/gallium/drivers/vc5/vc5_program.c b/src/gallium/drivers/vc5/vc5_program.c
index 4f902fd4c65d..2fb50897730d 100644
--- a/src/gallium/drivers/vc5/vc5_program.c
+++ b/src/gallium/drivers/vc5/vc5_program.c
@@ -378,6 +378,11 @@ vc5_update_compiled_fs(struct vc5_context *vc5, uint8_t prim_mode)
desc->channel[0].size == 32) {
key->f32_color_rb |= 1 << i;
}
+
+ if (desc->channel[0].pure_integer)
+ key->gl_fragcolor_lower_uvec4 |= 1 << i;
+ else
+ key->gl_fragcolor_lower_vec4 |= 1 << i;
}

if (key->is_points) {
--
2.15.0
Jason Ekstrand
2017-12-29 04:07:55 UTC
Reply
Permalink
Raw Message
Sure. Rb
Post by Eric Anholt
I needed this in the vc5 compiler.
---
src/compiler/nir_types.cpp | 6 ++++++
src/compiler/nir_types.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
index 377de0c9c7bd..cbdd452dc813 100644
--- a/src/compiler/nir_types.cpp
+++ b/src/compiler/nir_types.cpp
@@ -297,6 +297,12 @@ glsl_vec4_type(void)
return glsl_type::vec4_type;
}
+const glsl_type *
+glsl_uvec4_type(void)
+{
+ return glsl_type::uvec4_type;
+}
+
const glsl_type *
glsl_int_type(void)
{
diff --git a/src/compiler/nir_types.h b/src/compiler/nir_types.h
index daff97325093..4397c2406f9a 100644
--- a/src/compiler/nir_types.h
+++ b/src/compiler/nir_types.h
@@ -136,6 +136,7 @@ const struct glsl_type *glsl_double_type(void);
const struct glsl_type *glsl_vec_type(unsigned n);
const struct glsl_type *glsl_dvec_type(unsigned n);
const struct glsl_type *glsl_vec4_type(void);
+const struct glsl_type *glsl_uvec4_type(void);
const struct glsl_type *glsl_int_type(void);
const struct glsl_type *glsl_uint_type(void);
const struct glsl_type *glsl_int64_t_type(void);
--
2.15.0
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Kenneth Graunke
2017-12-29 08:00:58 UTC
Reply
Permalink
Raw Message
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
+ *
+ * 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 <***@whitecape.org>

though my review on your vc5 patch is a pretty low quality review :)
Eric Anholt
2017-12-29 22:48:13 UTC
Reply
Permalink
Raw Message
[ Unknown signature status ]
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?)
Yeah, it's gallium's internal glClearBufferiv() shader, translated using
TGSI-to-NIR. I had initially written this pass without the types
argument, but found that adding it got me a whole bunch more tests
working. I suppose we could move this into TTN and delay TTN until draw
time, to make sure that we always maintain proper types in the NIR, but
this was pretty easy. How about adding this to the top comment:

* The pass's types argument lets the caller choose between vec4, uvec4, and
* ivec4 per render target. This is necessary for TGSI-to-NIR output, which
* emits vec4 gl_FragColor writes even for integer render targets (because
* TGSI doesn't know the types).

If you use "out vec4 foo" but bind integer, you get "If the values
written by the fragment shader do not match the format(s) of the
corresponding color buffer(s), the result is undefined." (GL 4.3 spec)
+/**
+ * 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).
Agreed. I think I was just following core NIR.
+ 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
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.
How about:

/* We only emit 4-component stores above. If you need to lower to fewer
* components based on render target format, you probably need to do
* that in a separate pass anyway to cover the non-gl_FragColor case.
*/
assert(glsl_get_components(types[i]) == 4);
Kenneth Graunke
2017-12-29 22:59:58 UTC
Reply
Permalink
Raw Message
Post by Eric Anholt
[ Unknown signature status ]
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?)
Yeah, it's gallium's internal glClearBufferiv() shader, translated using
TGSI-to-NIR. I had initially written this pass without the types
argument, but found that adding it got me a whole bunch more tests
working. I suppose we could move this into TTN and delay TTN until draw
time, to make sure that we always maintain proper types in the NIR, but
* The pass's types argument lets the caller choose between vec4, uvec4, and
* ivec4 per render target. This is necessary for TGSI-to-NIR output, which
* emits vec4 gl_FragColor writes even for integer render targets (because
* TGSI doesn't know the types).
If you use "out vec4 foo" but bind integer, you get "If the values
written by the fragment shader do not match the format(s) of the
corresponding color buffer(s), the result is undefined." (GL 4.3 spec)
Ah cool, I had forgotten that bit of the spec :)

[snip]
Post by Eric Anholt
/* We only emit 4-component stores above. If you need to lower to fewer
* components based on render target format, you probably need to do
* that in a separate pass anyway to cover the non-gl_FragColor case.
*/
assert(glsl_get_components(types[i]) == 4);
Both of these comments sound good to me - I wouldn't bother with
anything more complicated. NIR shouldn't really care about types
either...it's effectively just going to be bitcast. You've got my:

Reviewed-by: Kenneth Graunke <***@whitecape.org>

Loading...