[Mesa-dev] [PATCH 06/92] nir/lower_system_values: add support for the FRAG_COORD system value

[Nicolai Hähnle]

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.

Cheers,
Nicolai


> 
>>
>> On 27/06/17 00:09, Nicolai Hähnle wrote:
>>> From: Nicolai Hähnle <nicolai.haehnle at 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 (cwabbott0 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


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