On 04/07/17 06:19, Connor Abbott wrote:
On Mon, Jul 3, 2017 at 8:51 AM, Nicolai Hähnle <nhaeh...@gmail.com> wrote:
On 03.07.2017 02:50, Timothy Arceri wrote:

On 03/07/17 10:46, Timothy Arceri wrote:

This and the previous patch seem a bit hacky to me. It seems it would be
better to either plum the system value all the way through NIR for all
drivers or simply disable GLSLFragCoordIsSysVal when radeonsi is going to be
using the NIR path.


Well maybe not disable, but surely we could ignore it.


I agree that it's a bit hacky, but having FragCoord as a system value is
cleaner more logical at least in radeonsi. Since this isn't necessarily true
for all drivers, I don't think it makes sense to do the "plumbing through
for all drivers".

Though I could change this patch to add a load_frag_coord NIR intrinsic
instead, that would be slightly cleaner.

Yeah, I don't know much about this, but if it's cleaner for radeonsi
(and presumably radv too?) for it to be a system value, then sure, add
the plumbing to make it a proper system value in NIR. Of course, other
drivers don't have to make it a system value though.

My concern here was that we are creating a system value in GLSL IR only to lower it in NIR so there seems no reason for it to be a system value in the first place. Having a path for both types through NIR seem excessive, and unhelpful. As far as I can tell RADV doesn't make use of it, please stop me if I'm wrong but the system values doesn't actually make it through NIR intact right?




Cheers,
Nicolai





On 27/06/17 00:09, Nicolai Hähnle wrote:

From: Nicolai Hähnle <nicolai.haeh...@amd.com>

Arguably this could convert to a load intrinsic, but other code paths
already expect this as a fragment shader input.
---
   src/compiler/nir/nir_lower_system_values.c | 36
++++++++++++++++++++++++++----
   1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/src/compiler/nir/nir_lower_system_values.c
b/src/compiler/nir/nir_lower_system_values.c
index 810100a..64a1354 100644
--- a/src/compiler/nir/nir_lower_system_values.c
+++ b/src/compiler/nir/nir_lower_system_values.c
@@ -21,22 +21,46 @@
    * IN THE SOFTWARE.
    *
    * Authors:
    *    Connor Abbott (cwabbo...@gmail.com)
    *
    */
   #include "nir.h"
   #include "nir_builder.h"
+static nir_ssa_def *
+get_frag_coord(nir_builder *b, nir_shader *shader, nir_variable *orig)
+{
+   nir_variable *pos = NULL;
+
+   assert(shader->stage == MESA_SHADER_FRAGMENT);
+
+   nir_foreach_variable(var, &shader->inputs) {
+      if (var->data.location == VARYING_SLOT_POS) {
+         pos = var;
+         break;
+      }
+   }
+
+   if (!pos) {
+      pos = nir_variable_create(shader, nir_var_shader_in,
glsl_vec4_type(), "gl_FragCoord");
+      pos->data.location = VARYING_SLOT_POS;
+      pos->data.pixel_center_integer = orig->data.pixel_center_integer;
+      pos->data.origin_upper_left = orig->data.origin_upper_left;
+   }
+
+   return nir_load_var(b, pos);
+}
+
   static bool
-convert_block(nir_block *block, nir_builder *b)
+convert_block(nir_block *block, nir_builder *b, nir_shader *shader)
   {
      bool progress = false;
      nir_foreach_instr_safe(instr, block) {
         if (instr->type != nir_instr_type_intrinsic)
            continue;
         nir_intrinsic_instr *load_var = nir_instr_as_intrinsic(instr);
         if (load_var->intrinsic != nir_intrinsic_load_var)
@@ -109,59 +133,63 @@ convert_block(nir_block *block, nir_builder *b)
               sysval = nir_load_vertex_id(b);
            }
            break;
         case SYSTEM_VALUE_INSTANCE_INDEX:
            sysval = nir_iadd(b,
                              nir_load_instance_id(b),
                              nir_load_base_instance(b));
            break;
+      case SYSTEM_VALUE_FRAG_COORD:
+         sysval = get_frag_coord(b, shader, var);
+         break;
+
         default:
            break;
         }
         if (sysval == NULL) {
            nir_intrinsic_op sysval_op =
               nir_intrinsic_from_system_value(var->data.location);
            sysval = nir_load_system_value(b, sysval_op, 0);
         }
         nir_ssa_def_rewrite_uses(&load_var->dest.ssa,
nir_src_for_ssa(sysval));
         nir_instr_remove(&load_var->instr);
         progress = true;
      }
      return progress;
   }
   static bool
-convert_impl(nir_function_impl *impl)
+convert_impl(nir_function_impl *impl, nir_shader *shader)
   {
      bool progress = false;
      nir_builder builder;
      nir_builder_init(&builder, impl);
      nir_foreach_block(block, impl) {
-      progress |= convert_block(block, &builder);
+      progress |= convert_block(block, &builder, shader);
      }
      nir_metadata_preserve(impl, nir_metadata_block_index |
                                  nir_metadata_dominance);
      return progress;
   }
   bool
   nir_lower_system_values(nir_shader *shader)
   {
      bool progress = false;
      nir_foreach_function(function, shader) {
         if (function->impl)
-         progress = convert_impl(function->impl) || progress;
+         progress = convert_impl(function->impl, shader) || progress;
      }
      exec_list_make_empty(&shader->system_values);
      return progress;
   }

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev



--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to