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

[Timothy Arceri]

On 04/07/17 06:19, Connor Abbott wrote:
> 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.

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.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