[Mesa-dev] [PATCH 06/92] nir/lower_system_values: add support for the FRAG_COORD system value
Connor Abbott
cwabbott0 at gmail.com
Mon Jul 3 21:57:46 UTC 2017
On Mon, Jul 3, 2017 at 2:34 PM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
> 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?
radv doesn't handle it as a system value, but it would be better if it
would -- from a quick search, FragCoord seems to be handled
differently from all the other inputs, since it gets its own dedicated
VGPR's. IIRC we already have two different paths through NIR for other
things, and it isn't excessive or unhelpful; quite to the contrary, it
simplifies the driver logic. It complicates the lowering passes, but
that's ok, since the entire point of the lowering passes is to get as
much logic out of the driver as possible.
>
>
>
>>
>>>
>>> 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
More information about the mesa-dev
mailing list