[4/7] nir: Add nir_lower_blend pass

Submitted by Alyssa Rosenzweig on May 6, 2019, 2:26 a.m.

Details

Message ID 20190506022611.4954-5-alyssa@rosenzweig.io
State New
Headers show
Series "Blend shaders! (NIR lowering pass)" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alyssa Rosenzweig May 6, 2019, 2:26 a.m.
This new lowering pass implements the OpenGL ES blend pipeline in
shaders, applicable to hardware lacking full-featured blending hardware
(including Midgard/Bifrost and vc4). This pass is run on a fragment
shader, rewriting the store to a blended version, loading in the
framebuffer destination color and constant color via intrinsics as
necessary. This pass is sufficient for OpenGL ES 2.0 and is verified to
pass dEQP's blend tests. That said, at present it has the following
limitations:

 - MRT is not supported.
 - Logic ops are not supported.

MRT support is on my TODO list but paused until MRT is implemented in
the rest of the driver. Both changes should be fairly trivial.

It also includes MIN/MAX modes, so in conjunction with the advanced
blend mode lowering it should be sufficient for ES3, though this has not
been thoroughly tested. It is an open question whether the current GLSL
IR based advanced blend lowering should be NIRified and merged into this
pass.

 ...Dual-source blending is not supported, Ryan.

Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Cc: Eric Anholt <eric@anholt.net>
Cc: Kenneth Graunke <kenneth@whitecape.org>
---
 src/compiler/Makefile.sources      |   1 +
 src/compiler/nir/meson.build       |   1 +
 src/compiler/nir/nir.h             |  22 +++
 src/compiler/nir/nir_lower_blend.c | 214 +++++++++++++++++++++++++++++
 4 files changed, 238 insertions(+)
 create mode 100644 src/compiler/nir/nir_lower_blend.c

Patch hide | download patch | download mbox

diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
index 9bebc3d8867..d68b9550b02 100644
--- a/src/compiler/Makefile.sources
+++ b/src/compiler/Makefile.sources
@@ -238,6 +238,7 @@  NIR_FILES = \
 	nir/nir_lower_bit_size.c \
 	nir/nir_lower_bool_to_float.c \
 	nir/nir_lower_bool_to_int32.c \
+	nir/nir_lower_blend.c \
 	nir/nir_lower_clamp_color_outputs.c \
 	nir/nir_lower_clip.c \
 	nir/nir_lower_clip_cull_distance_arrays.c \
diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
index a8faeb9c018..73ab62d4b46 100644
--- a/src/compiler/nir/meson.build
+++ b/src/compiler/nir/meson.build
@@ -116,6 +116,7 @@  files_libnir = files(
   'nir_lower_array_deref_of_vec.c',
   'nir_lower_atomics_to_ssbo.c',
   'nir_lower_bitmap.c',
+  'nir_lower_blend.c',
   'nir_lower_bool_to_float.c',
   'nir_lower_bool_to_int32.c',
   'nir_lower_clamp_color_outputs.c',
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 37161e83e4d..8b68faed819 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -3447,6 +3447,28 @@  typedef enum  {
 
 bool nir_lower_to_source_mods(nir_shader *shader, nir_lower_to_source_mods_flags options);
 
+/* These structs encapsulates the blend state such that it can be lowered
+ * cleanly */
+
+typedef struct {
+      enum blend_func func;
+
+      enum blend_factor src_factor;
+      bool invert_src_factor;
+
+      enum blend_factor dst_factor;
+      bool invert_dst_factor;
+} nir_lower_blend_channel;
+
+typedef struct {
+   struct {
+      nir_lower_blend_channel rgb;
+      nir_lower_blend_channel alpha;
+   } rt[8];
+} nir_lower_blend_options;
+
+void nir_lower_blend(nir_shader *shader, nir_lower_blend_options options);
+
 bool nir_lower_gs_intrinsics(nir_shader *shader);
 
 typedef unsigned (*nir_lower_bit_size_callback)(const nir_alu_instr *, void *);
diff --git a/src/compiler/nir/nir_lower_blend.c b/src/compiler/nir/nir_lower_blend.c
new file mode 100644
index 00000000000..5a874f08834
--- /dev/null
+++ b/src/compiler/nir/nir_lower_blend.c
@@ -0,0 +1,214 @@ 
+/*
+ * Copyright (C) 2019 Alyssa Rosenzweig
+ *
+ * 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.
+ */
+
+/**
+ * @file
+ *
+ * Implements the fragment pipeline (blending and writeout) in software, to be
+ * run as a dedicated "blend shader" stage on Midgard/Bifrost, or as a fragment
+ * shader variant on typical GPUs. This pass is useful if hardware lacks
+ * fixed-function blending in part or in full.
+ */
+
+#include "nir/nir.h"
+#include "nir/nir_builder.h"
+
+/* Given processed factors, combine them per a blend function */
+
+static nir_ssa_def *
+nir_blend_func(
+      nir_builder *b,
+      enum blend_func func,
+      nir_ssa_def *src, nir_ssa_def *dst)
+{
+   switch (func) {
+      case BLEND_FUNC_ADD:
+         return nir_fadd(b, src, dst);
+      case BLEND_FUNC_SUBTRACT:
+         return nir_fsub(b, src, dst);
+      case BLEND_FUNC_REVERSE_SUBTRACT:
+         return nir_fsub(b, dst, src);
+      case BLEND_FUNC_MIN:
+         return nir_fmin(b, src, dst);
+      case BLEND_FUNC_MAX:
+         return nir_fmax(b, src, dst);
+      default:
+         unreachable("Invalid blend function");
+         break;
+   }
+}
+
+/* Does this blend function multiply by a blend factor? */
+
+static bool
+nir_blend_factored(enum blend_func func) {
+   switch (func) {
+      case BLEND_FUNC_ADD:
+      case BLEND_FUNC_SUBTRACT:
+      case BLEND_FUNC_REVERSE_SUBTRACT:
+         return true;
+      default:
+         return false;
+   }
+}
+
+/* Returns a scalar single factor, unmultiplied */
+
+static nir_ssa_def *
+nir_blend_factor_value(
+      nir_builder *b,
+      nir_ssa_def *src, nir_ssa_def *dst, nir_ssa_def *bconst,
+      unsigned chan,
+      enum blend_factor factor)
+{
+   switch (factor) {
+      case BLEND_FACTOR_ZERO:
+         return nir_imm_float(b, 0.0);
+      case BLEND_FACTOR_SRC_COLOR:
+         return nir_channel(b, src, chan);
+      case BLEND_FACTOR_DST_COLOR:
+         return nir_channel(b, dst, chan);
+      case BLEND_FACTOR_SRC_ALPHA:
+         return nir_channel(b, src, 3);
+      case BLEND_FACTOR_DST_ALPHA:
+         return nir_channel(b, dst, 3);
+      case BLEND_FACTOR_CONSTANT_COLOR:
+         return nir_channel(b, bconst, chan);
+      case BLEND_FACTOR_CONSTANT_ALPHA:
+         return nir_channel(b, bconst, 3);
+      case BLEND_FACTOR_SRC_ALPHA_SATURATE: {
+         nir_ssa_def *Asrc = nir_channel(b, src, 3);
+         nir_ssa_def *Adst = nir_channel(b, dst, 3);
+         nir_ssa_def *one = nir_imm_float(b, 1.0);
+         nir_ssa_def *Adsti = nir_fsub(b, one, Adst);
+
+         return (chan < 3) ? nir_fmin(b, Asrc, Adsti) : one;
+      }
+      default:
+         unreachable("Invalid blend factor");
+   }
+}
+
+static nir_ssa_def *
+nir_blend_factor(
+      nir_builder *b,
+      nir_ssa_def *raw_scalar,
+      nir_ssa_def *src, nir_ssa_def *dst, nir_ssa_def *bconst,
+      unsigned chan,
+      enum blend_factor factor,
+      bool inverted)
+{
+   nir_ssa_def *f =
+      nir_blend_factor_value(b, src, dst, bconst, chan, factor);
+
+   if (inverted)
+      f = nir_fsub(b, nir_imm_float(b, 1.0), f);
+
+   return nir_fmul(b, raw_scalar, f);
+}
+
+/* Given a blend state, the source color, and the destination color,
+ * return the blended color */
+
+static nir_ssa_def *
+nir_blend(
+      nir_builder *b,
+      nir_lower_blend_options options,
+      nir_ssa_def *src, nir_ssa_def *dst)
+{
+   /* Grab the blend constant ahead of time */
+   nir_ssa_def *bconst = nir_load_blend_const_color_rgba(b);
+
+   /* We blend per channel and recombine later */
+   nir_ssa_def *channels[4];
+
+   for (unsigned c = 0; c < 4; ++c) {
+      /* Decide properties based on channel */
+      nir_lower_blend_channel chan =
+         (c < 3) ? options.rt[0].rgb : options.rt[0].alpha;
+
+      nir_ssa_def *psrc = nir_channel(b, src, c);
+      nir_ssa_def *pdst = nir_channel(b, dst, c);
+
+      if (nir_blend_factored(chan.func)) {
+         psrc = nir_blend_factor(
+                  b, psrc,
+                  src, dst, bconst, c,
+                  chan.src_factor, chan.invert_src_factor);
+
+         pdst = nir_blend_factor(
+                  b, pdst,
+                  src, dst, bconst, c,
+                  chan.dst_factor, chan.invert_dst_factor);
+      }
+
+      channels[c] = nir_blend_func(b, chan.func, psrc, pdst);
+   }
+
+   return nir_vec(b, channels, 4);
+}
+
+void
+nir_lower_blend(nir_shader *shader, nir_lower_blend_options options)
+{
+   /* Blend shaders are represented as special fragment shaders */
+   assert(shader->info.stage == MESA_SHADER_FRAGMENT);
+
+   nir_foreach_function(func, shader) {
+      nir_foreach_block(block, func->impl) {
+         nir_foreach_instr_safe(instr, block) {
+            if (instr->type != nir_instr_type_intrinsic)
+               continue;
+
+            nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
+            if (intr->intrinsic != nir_intrinsic_store_deref)
+               continue;
+
+            /* TODO: Extending to MRT */
+            nir_variable *var = nir_intrinsic_get_var(intr, 0);
+            if (var->data.location != FRAG_RESULT_COLOR)
+               continue;
+
+            nir_builder b;
+            nir_builder_init(&b, func->impl);
+            b.cursor = nir_before_instr(instr);
+
+            /* Grab the input color */
+            nir_ssa_def *src = nir_ssa_for_src(&b, intr->src[1], 4);
+
+            /* Grab the tilebuffer color - io lowered to load_output */
+            nir_ssa_def *dst = nir_load_var(&b, var);
+
+            /* Blend and write that instead */
+            nir_ssa_def *blended = nir_blend(&b, options, src, dst);
+
+            nir_instr_rewrite_src(instr, &intr->src[1],
+                                  nir_src_for_ssa(blended));
+ 
+         }
+      }
+
+      nir_metadata_preserve(func->impl, nir_metadata_block_index |
+                            nir_metadata_dominance);
+   }
+}

Comments


On 5/5/19 7:26 PM, Alyssa Rosenzweig wrote:
> This new lowering pass implements the OpenGL ES blend pipeline in
> shaders, applicable to hardware lacking full-featured blending hardware
> (including Midgard/Bifrost and vc4). This pass is run on a fragment
> shader, rewriting the store to a blended version, loading in the
> framebuffer destination color and constant color via intrinsics as
> necessary. This pass is sufficient for OpenGL ES 2.0 and is verified to
> pass dEQP's blend tests. That said, at present it has the following
> limitations:
> 
>  - MRT is not supported.
>  - Logic ops are not supported.

Logic ops seem... challenging to emulate in the shader.  That shader
would need the destination colors in the framebuffer storage format, and
I'm not sure that's always possible (maybe?).  We (and I think everyone
else) removed GL_EXT_blend_logic_op because nobody's hardware could
handle the interactions GL_EXT_blend_equation_separate.  It would be
cool to add it back. :)

It might also be fun to add support for GL_AMD_blend_minmax_factor and
GL_SGIX_blend_alpha_minmax.  Looking at this lowering pass, it seems
like most of the work would be adding tests.

> MRT support is on my TODO list but paused until MRT is implemented in
> the rest of the driver. Both changes should be fairly trivial.
> 
> It also includes MIN/MAX modes, so in conjunction with the advanced
> blend mode lowering it should be sufficient for ES3, though this has not
> been thoroughly tested. It is an open question whether the current GLSL
> IR based advanced blend lowering should be NIRified and merged into this
> pass.

Having all of the lowering related to blending in one place seems like a
good idea.

>  ...Dual-source blending is not supported, Ryan.
> 
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Kenneth Graunke <kenneth@whitecape.org>
> ---
>  src/compiler/Makefile.sources      |   1 +
>  src/compiler/nir/meson.build       |   1 +
>  src/compiler/nir/nir.h             |  22 +++
>  src/compiler/nir/nir_lower_blend.c | 214 +++++++++++++++++++++++++++++
>  4 files changed, 238 insertions(+)
>  create mode 100644 src/compiler/nir/nir_lower_blend.c
> 
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index 9bebc3d8867..d68b9550b02 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -238,6 +238,7 @@ NIR_FILES = \
>  	nir/nir_lower_bit_size.c \
>  	nir/nir_lower_bool_to_float.c \
>  	nir/nir_lower_bool_to_int32.c \
> +	nir/nir_lower_blend.c \
>  	nir/nir_lower_clamp_color_outputs.c \
>  	nir/nir_lower_clip.c \
>  	nir/nir_lower_clip_cull_distance_arrays.c \
> diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
> index a8faeb9c018..73ab62d4b46 100644
> --- a/src/compiler/nir/meson.build
> +++ b/src/compiler/nir/meson.build
> @@ -116,6 +116,7 @@ files_libnir = files(
>    'nir_lower_array_deref_of_vec.c',
>    'nir_lower_atomics_to_ssbo.c',
>    'nir_lower_bitmap.c',
> +  'nir_lower_blend.c',
>    'nir_lower_bool_to_float.c',
>    'nir_lower_bool_to_int32.c',
>    'nir_lower_clamp_color_outputs.c',
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 37161e83e4d..8b68faed819 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -3447,6 +3447,28 @@ typedef enum  {
>  
>  bool nir_lower_to_source_mods(nir_shader *shader, nir_lower_to_source_mods_flags options);
>  
> +/* These structs encapsulates the blend state such that it can be lowered
                    encapsulate
> + * cleanly */

*/ on its own line.  There's at least one more instance of this below.

> +
> +typedef struct {
> +      enum blend_func func;
> +
> +      enum blend_factor src_factor;
> +      bool invert_src_factor;
> +
> +      enum blend_factor dst_factor;
> +      bool invert_dst_factor;
> +} nir_lower_blend_channel;
> +
> +typedef struct {
> +   struct {
> +      nir_lower_blend_channel rgb;
> +      nir_lower_blend_channel alpha;
> +   } rt[8];
> +} nir_lower_blend_options;
> +
> +void nir_lower_blend(nir_shader *shader, nir_lower_blend_options options);
> +
>  bool nir_lower_gs_intrinsics(nir_shader *shader);
>  
>  typedef unsigned (*nir_lower_bit_size_callback)(const nir_alu_instr *, void *);
> diff --git a/src/compiler/nir/nir_lower_blend.c b/src/compiler/nir/nir_lower_blend.c
> new file mode 100644
> index 00000000000..5a874f08834
> --- /dev/null
> +++ b/src/compiler/nir/nir_lower_blend.c
> @@ -0,0 +1,214 @@
> +/*
> + * Copyright (C) 2019 Alyssa Rosenzweig
> + *
> + * 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.
> + */
> +
> +/**
> + * @file
> + *
> + * Implements the fragment pipeline (blending and writeout) in software, to be
> + * run as a dedicated "blend shader" stage on Midgard/Bifrost, or as a fragment
> + * shader variant on typical GPUs. This pass is useful if hardware lacks
> + * fixed-function blending in part or in full.
> + */
> +
> +#include "nir/nir.h"
> +#include "nir/nir_builder.h"
> +
> +/* Given processed factors, combine them per a blend function */
> +
> +static nir_ssa_def *
> +nir_blend_func(
> +      nir_builder *b,
> +      enum blend_func func,
> +      nir_ssa_def *src, nir_ssa_def *dst)
> +{
> +   switch (func) {
> +      case BLEND_FUNC_ADD:

Alas, case should be at the same indentation level as switch.

> +         return nir_fadd(b, src, dst);
> +      case BLEND_FUNC_SUBTRACT:
> +         return nir_fsub(b, src, dst);
> +      case BLEND_FUNC_REVERSE_SUBTRACT:
> +         return nir_fsub(b, dst, src);
> +      case BLEND_FUNC_MIN:
> +         return nir_fmin(b, src, dst);
> +      case BLEND_FUNC_MAX:
> +         return nir_fmax(b, src, dst);
> +      default:
> +         unreachable("Invalid blend function");
> +         break;

Since factor is an enum, I think it's better to not have the default
case.  If there's no default case, the compiler will issue a warning if
a new enum is added but not handled.

Either way, the break is definitely not necessary.

> +   }
> +}
> +
> +/* Does this blend function multiply by a blend factor? */
> +
> +static bool
> +nir_blend_factored(enum blend_func func) {

{ goes on it's own line.

> +   switch (func) {
> +      case BLEND_FUNC_ADD:
> +      case BLEND_FUNC_SUBTRACT:
> +      case BLEND_FUNC_REVERSE_SUBTRACT:
> +         return true;
> +      default:
> +         return false;
> +   }
> +}
> +
> +/* Returns a scalar single factor, unmultiplied */
> +
> +static nir_ssa_def *
> +nir_blend_factor_value(
> +      nir_builder *b,
> +      nir_ssa_def *src, nir_ssa_def *dst, nir_ssa_def *bconst,
> +      unsigned chan,
> +      enum blend_factor factor)
> +{
> +   switch (factor) {
> +      case BLEND_FACTOR_ZERO:
> +         return nir_imm_float(b, 0.0);
> +      case BLEND_FACTOR_SRC_COLOR:
> +         return nir_channel(b, src, chan);
> +      case BLEND_FACTOR_DST_COLOR:
> +         return nir_channel(b, dst, chan);
> +      case BLEND_FACTOR_SRC_ALPHA:
> +         return nir_channel(b, src, 3);
> +      case BLEND_FACTOR_DST_ALPHA:
> +         return nir_channel(b, dst, 3);
> +      case BLEND_FACTOR_CONSTANT_COLOR:
> +         return nir_channel(b, bconst, chan);
> +      case BLEND_FACTOR_CONSTANT_ALPHA:
> +         return nir_channel(b, bconst, 3);
> +      case BLEND_FACTOR_SRC_ALPHA_SATURATE: {
> +         nir_ssa_def *Asrc = nir_channel(b, src, 3);
> +         nir_ssa_def *Adst = nir_channel(b, dst, 3);
> +         nir_ssa_def *one = nir_imm_float(b, 1.0);
> +         nir_ssa_def *Adsti = nir_fsub(b, one, Adst);
> +
> +         return (chan < 3) ? nir_fmin(b, Asrc, Adsti) : one;
> +      }
> +      default:
> +         unreachable("Invalid blend factor");

Same default case comment here as above.

> +   }
> +}
> +
> +static nir_ssa_def *
> +nir_blend_factor(
> +      nir_builder *b,
> +      nir_ssa_def *raw_scalar,
> +      nir_ssa_def *src, nir_ssa_def *dst, nir_ssa_def *bconst,
> +      unsigned chan,
> +      enum blend_factor factor,
> +      bool inverted)
> +{
> +   nir_ssa_def *f =
> +      nir_blend_factor_value(b, src, dst, bconst, chan, factor);
> +
> +   if (inverted)
> +      f = nir_fsub(b, nir_imm_float(b, 1.0), f);
> +
> +   return nir_fmul(b, raw_scalar, f);
> +}
> +
> +/* Given a blend state, the source color, and the destination color,
> + * return the blended color */
> +
> +static nir_ssa_def *
> +nir_blend(
> +      nir_builder *b,
> +      nir_lower_blend_options options,
> +      nir_ssa_def *src, nir_ssa_def *dst)
> +{
> +   /* Grab the blend constant ahead of time */
> +   nir_ssa_def *bconst = nir_load_blend_const_color_rgba(b);
> +
> +   /* We blend per channel and recombine later */

Why?  Without a vectorizer (or even with a vectorizer), it seems like
this will generate much worse code.  I guess it's only a few
instructions once per shader, so it may not matter... but it's a little
surprising especially after going to extra effort to get the blend color
as a vector.

> +   nir_ssa_def *channels[4];
> +
> +   for (unsigned c = 0; c < 4; ++c) {
> +      /* Decide properties based on channel */
> +      nir_lower_blend_channel chan =
> +         (c < 3) ? options.rt[0].rgb : options.rt[0].alpha;
> +
> +      nir_ssa_def *psrc = nir_channel(b, src, c);
> +      nir_ssa_def *pdst = nir_channel(b, dst, c);
> +
> +      if (nir_blend_factored(chan.func)) {
> +         psrc = nir_blend_factor(
> +                  b, psrc,
> +                  src, dst, bconst, c,
> +                  chan.src_factor, chan.invert_src_factor);
> +
> +         pdst = nir_blend_factor(
> +                  b, pdst,
> +                  src, dst, bconst, c,
> +                  chan.dst_factor, chan.invert_dst_factor);
> +      }
> +
> +      channels[c] = nir_blend_func(b, chan.func, psrc, pdst);
> +   }
> +
> +   return nir_vec(b, channels, 4);
> +}
> +
> +void
> +nir_lower_blend(nir_shader *shader, nir_lower_blend_options options)
> +{
> +   /* Blend shaders are represented as special fragment shaders */
> +   assert(shader->info.stage == MESA_SHADER_FRAGMENT);
> +
> +   nir_foreach_function(func, shader) {
> +      nir_foreach_block(block, func->impl) {
> +         nir_foreach_instr_safe(instr, block) {

FWIW, since someone will probably comment, I like this organization
better than the method other passes use of splitting things into
multiple functions.

> +            if (instr->type != nir_instr_type_intrinsic)
> +               continue;
> +
> +            nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
> +            if (intr->intrinsic != nir_intrinsic_store_deref)
> +               continue;
> +
> +            /* TODO: Extending to MRT */
> +            nir_variable *var = nir_intrinsic_get_var(intr, 0);
> +            if (var->data.location != FRAG_RESULT_COLOR)
> +               continue;
> +
> +            nir_builder b;
> +            nir_builder_init(&b, func->impl);
> +            b.cursor = nir_before_instr(instr);
> +
> +            /* Grab the input color */
> +            nir_ssa_def *src = nir_ssa_for_src(&b, intr->src[1], 4);
> +
> +            /* Grab the tilebuffer color - io lowered to load_output */
> +            nir_ssa_def *dst = nir_load_var(&b, var);
> +
> +            /* Blend and write that instead */
> +            nir_ssa_def *blended = nir_blend(&b, options, src, dst);
> +
> +            nir_instr_rewrite_src(instr, &intr->src[1],
> +                                  nir_src_for_ssa(blended));
> + 
> +         }
> +      }
> +
> +      nir_metadata_preserve(func->impl, nir_metadata_block_index |
> +                            nir_metadata_dominance);
> +   }
> +}
> Logic ops seem... challenging to emulate in the shader.  That shader
> would need the destination colors in the framebuffer storage format, and
> I'm not sure that's always possible (maybe?). 

Alright, that's good to know.

I will note that in Midgard, the native hardware ops are to load/store
the actual framebuffer storage format. As seen later in the series, I
lower load/store_output to a series of ops converting to/from float and
the actual format. It's an open-question whether we want actual
nir_intrinsics for this so the lowering happens in common NIR code and
then we get fun logic ops and such.

> It might also be fun to add support for GL_AMD_blend_minmax_factor and
> GL_SGIX_blend_alpha_minmax.  Looking at this lowering pass, it seems
> like most of the work would be adding tests.

Probably!

> Having all of the lowering related to blending in one place seems like a
> good idea.

Probably, yes. Also since GLSL IR is, well.... ;)

> > +/* These structs encapsulates the blend state such that it can be lowered
>                     encapsulate
> > + * cleanly */
> 
> */ on its own line.  There's at least one more instance of this below.

Is there, uh, a thing I can stick in my vimrc to get this right?

> Alas, case should be at the same indentation level as switch.

Good to know, thank you.

> Since factor is an enum, I think it's better to not have the default
> case.  If there's no default case, the compiler will issue a warning if
> a new enum is added but not handled.
> 
> Either way, the break is definitely not necessary.

+1

> { goes on it's own line.

--Wait, that's a rule I actually follow, how'd I mess it up here? :P

> Why?  Without a vectorizer (or even with a vectorizer), it seems like
> this will generate much worse code.  I guess it's only a few
> instructions once per shader, so it may not matter... but it's a little
> surprising especially after going to extra effort to get the blend color
> as a vector.

Honestly? Since that's how vc4 (scalar) did it and for v1 of this series I was
more eager to get something passing tests than particularly good. In my
original implementation (before joining upstream), I did it like this
and then used nir_opt_vectorize to get it back to something reasonable.
That's one strategy still. The other option is to try to generate vector
out of the gate, but that's hard to get right when RGB/alpha can be
separate (but don't have to be), etc. The logic needed would approach
just, well, having ALU vectorization... might as well merge the
vectorize pass at that point...

Suggestions welcome :)

> FWIW, since someone will probably comment, I like this organization
> better than the method other passes use of splitting things into
> multiple functions.

Hehe, thank you :)