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

[Connor Abbott]

On Mon, Jul 3, 2017 at 8:51 AM, Nicolai Hähnle <nhaehnle at 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.

>
> 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.
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev