intel/nir: Rewrite the guts of lower_alpha_to_coverage

I have no idea how this pass ever worked.  I guess it worked ok on the
one or two piglit tests but the whole thing seemed very fragile.  It
makes a number of undocumented and unasserted assumptions and they
aren't always valid.  This rewrite makes a number of changes:

 1. It now properly handles the case where the gl_SampleMask write comes
    before the gl_FragColor or gl_FragData[0] write.

 2. It should early-exit faster because it now looks at bits in
    shader_info::outputs_written instead of looking for variables.

 3. Instead of the fragile variable lookup where we try to look the
    variable up by both location and driver_location and match, we just
    use the driver_location calculations used by brw_fs_nir.

 4. It asserts that the index parameter to store_output is a constant
    instead of silently failing if it isn't.

 5. We now actually assert the implicit assumption that the two writes
    are in the same block.  We go even further and assert that they are
    in the last block in the shader.

 6. In the case where 3 or fewer components of the output are written,
    we explicitly choose to leave the sample mask alone.

Fixes: 7ecfbd4f6d4 "nir: Add alpha_to_coverage lowering pass"
Closes: #3166
Reviewed-by: Caio Marcelo de Oliveira Filho <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6233>

diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h
index 1241e54fc5a150f1c1d1d584d23d197de97c476a..616c424429580c840a33585feb9d25586e9b1d6b 100644 (file)
--- a/src/intel/compiler/brw_nir.h
+++ b/src/intel/compiler/brw_nir.h
@@ -100,7 +100,7 @@ brw_nir_link_shaders(const struct brw_compiler *compiler,
                      nir_shader *producer, nir_shader *consumer);
 
 bool brw_nir_lower_cs_intrinsics(nir_shader *nir);
-void brw_nir_lower_alpha_to_coverage(nir_shader *shader);
+bool brw_nir_lower_alpha_to_coverage(nir_shader *shader);
 void brw_nir_lower_legacy_clipping(nir_shader *nir,
                                    int nr_userclip_plane_consts,
                                    struct brw_stage_prog_data *prog_data);

diff --git a/src/intel/compiler/brw_nir_lower_alpha_to_coverage.c b/src/intel/compiler/brw_nir_lower_alpha_to_coverage.c
index c804bdeaab835e8c1415589b1f67946951ce770b..d31384e5615cfaf9275375b888d4296da055b382 100644 (file)
--- a/src/intel/compiler/brw_nir_lower_alpha_to_coverage.c
+++ b/src/intel/compiler/brw_nir_lower_alpha_to_coverage.c
@@ -56,10 +56,10 @@
  *  1.0000 1111111111111111
  */
 static nir_ssa_def *
-build_dither_mask(nir_builder *b, nir_intrinsic_instr *store_instr)
+build_dither_mask(nir_builder *b, nir_ssa_def *color)
 {
-   nir_ssa_def *alpha =
-      nir_channel(b, nir_ssa_for_src(b, store_instr->src[0], 4), 3);
+   assert(color->num_components == 4);
+   nir_ssa_def *alpha = nir_channel(b, color, 3);
 
    nir_ssa_def *m =
       nir_f2i32(b, nir_fmul_imm(b, nir_fsat(b, alpha), 16.0));
@@ -82,74 +82,108 @@ build_dither_mask(nir_builder *b, nir_intrinsic_instr *store_instr)
                           nir_imul_imm(b, part_c, 0x0100)));
 }
 
-void
+bool
 brw_nir_lower_alpha_to_coverage(nir_shader *shader)
 {
    assert(shader->info.stage == MESA_SHADER_FRAGMENT);
-
-   /* Bail out early if we don't have gl_SampleMask */
-   if (!nir_find_variable_with_location(shader, nir_var_shader_out,
-                                        FRAG_RESULT_SAMPLE_MASK))
-      return;
-
-   nir_foreach_function(function, shader) {
-      nir_function_impl *impl = function->impl;
-      nir_builder b;
-      nir_builder_init(&b, impl);
-
-      nir_foreach_block(block, impl) {
-         nir_intrinsic_instr *sample_mask_instr = NULL;
-         nir_intrinsic_instr *store_instr = NULL;
-
-         nir_foreach_instr_safe(instr, block) {
-            if (instr->type == nir_instr_type_intrinsic) {
-               nir_intrinsic_instr *intr = nir_instr_as_intrinsic(instr);
-               nir_variable *out = NULL;
-
-               switch (intr->intrinsic) {
-               case nir_intrinsic_store_output:
-                  out = nir_find_variable_with_driver_location(shader, nir_var_shader_out,
-                                                               nir_intrinsic_base(intr));
-                  assert(out->data.mode == nir_var_shader_out);
-
-                  /* save gl_SampleMask instruction pointer */
-                  if (out->data.location == FRAG_RESULT_SAMPLE_MASK) {
-                     assert(!sample_mask_instr);
-                     sample_mask_instr = intr;
-                  }
-
-                  /* save out_color[0] instruction pointer */
-                  if ((out->data.location == FRAG_RESULT_COLOR ||
-                      out->data.location == FRAG_RESULT_DATA0)) {
-                     nir_src *offset_src = nir_get_io_offset_src(intr);
-                     if (nir_src_is_const(*offset_src) && nir_src_as_uint(*offset_src) == 0) {
-                        assert(!store_instr);
-                        store_instr = intr;
-                     }
-                  }
-                  break;
-               default:
-                  continue;
-               }
-            }
+   nir_function_impl *impl = nir_shader_get_entrypoint(shader);
+
+   const uint64_t outputs_written = shader->info.outputs_written;
+   if (!(outputs_written & BITFIELD64_BIT(FRAG_RESULT_SAMPLE_MASK)) ||
+       !(outputs_written & (BITFIELD64_BIT(FRAG_RESULT_COLOR) |
+                            BITFIELD64_BIT(FRAG_RESULT_DATA0))))
+      goto skip;
+
+   nir_intrinsic_instr *sample_mask_write = NULL;
+   nir_intrinsic_instr *color0_write = NULL;
+   bool sample_mask_write_first = false;
+
+   nir_foreach_block(block, impl) {
+      nir_foreach_instr_safe(instr, block) {
+         if (instr->type != nir_instr_type_intrinsic)
+            continue;
+
+         nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
+         if (intrin->intrinsic != nir_intrinsic_store_output)
+            continue;
+
+         /* We call nir_lower_io_to_temporaries to lower FS outputs to
+          * temporaries with a copy at the end so this should be the last
+          * block in the shader.
+          */
+         assert(block->cf_node.parent == &impl->cf_node);
+         assert(nir_cf_node_is_last(&block->cf_node));
+
+         /* See store_output in fs_visitor::nir_emit_fs_intrinsic */
+         const unsigned store_offset = nir_src_as_uint(intrin->src[1]);
+         const unsigned driver_location = nir_intrinsic_base(intrin) +
+            SET_FIELD(store_offset, BRW_NIR_FRAG_OUTPUT_LOCATION);
+
+         /* Extract the FRAG_RESULT */
+         const unsigned location =
+            GET_FIELD(driver_location, BRW_NIR_FRAG_OUTPUT_LOCATION);
+
+         if (location == FRAG_RESULT_SAMPLE_MASK) {
+            assert(sample_mask_write == NULL);
+            sample_mask_write = intrin;
+            sample_mask_write_first = (color0_write == NULL);
          }
 
-         if (sample_mask_instr && store_instr) {
-            b.cursor = nir_before_instr(&store_instr->instr);
-            nir_ssa_def *dither_mask = build_dither_mask(&b, store_instr);
-
-            /* Combine dither_mask and reorder gl_SampleMask store instruction
-             * after render target 0 store instruction.
-             */
-            nir_instr_remove(&sample_mask_instr->instr);
-            dither_mask = nir_iand(&b, sample_mask_instr->src[0].ssa, dither_mask);
-            nir_instr_insert_after(&store_instr->instr, &sample_mask_instr->instr);
-            nir_instr_rewrite_src(&sample_mask_instr->instr,
-                                  &sample_mask_instr->src[0],
-                                  nir_src_for_ssa(dither_mask));
+         if (location == FRAG_RESULT_COLOR ||
+             location == FRAG_RESULT_DATA0) {
+            assert(color0_write == NULL);
+            color0_write = intrin;
          }
       }
-      nir_metadata_preserve(impl, nir_metadata_block_index |
-                            nir_metadata_dominance);
    }
+
+   /* It's possible that shader_info may be out-of-date and the writes to
+    * either gl_SampleMask or the first color value may have been removed.
+    * This can happen if, for instance a nir_ssa_undef is written to the
+    * color value.  In that case, just bail and don't do anything rather
+    * than crashing.
+    */
+   if (color0_write == NULL || sample_mask_write == NULL)
+      goto skip;
+
+   /* It's possible that the color value isn't actually a vec4.  In this case,
+    * assuming an alpha of 1.0 and letting the sample mask pass through
+    * unaltered seems like the kindest thing to do to apps.
+    */
+   assert(color0_write->src[0].is_ssa);
+   nir_ssa_def *color0 = color0_write->src[0].ssa;
+   if (color0->num_components < 4)
+      goto skip;
+
+   assert(sample_mask_write->src[0].is_ssa);
+   nir_ssa_def *sample_mask = sample_mask_write->src[0].ssa;
+
+   if (sample_mask_write_first) {
+      /* If the sample mask write comes before the write to color0, we need
+       * to move it because it's going to use the value from color0 to
+       * compute the sample mask.
+       */
+      nir_instr_remove(&sample_mask_write->instr);
+      nir_instr_insert(nir_after_instr(&color0_write->instr),
+                       &sample_mask_write->instr);
+   }
+
+   nir_builder b;
+   nir_builder_init(&b, impl);
+
+   /* Combine dither_mask and the gl_SampleMask value */
+   b.cursor = nir_before_instr(&sample_mask_write->instr);
+   nir_ssa_def *dither_mask = build_dither_mask(&b, color0);
+   dither_mask = nir_iand(&b, sample_mask, dither_mask);
+   nir_instr_rewrite_src(&sample_mask_write->instr,
+                         &sample_mask_write->src[0],
+                         nir_src_for_ssa(dither_mask));
+
+   nir_metadata_preserve(impl, nir_metadata_block_index |
+                               nir_metadata_dominance);
+   return true;
+
+skip:
+   nir_metadata_preserve(impl, nir_metadata_all);
+   return false;
 }