[Mesa-dev] [PATCH 2/3] radeonsi/nir: Set vs_inputs_dual_locations and let NIR do the remap

[Jason Ekstrand]

On September 5, 2018 19:46:06 Timothy Arceri <tarceri at itsqueeze.com> wrote:

> On 01/09/18 13:11, Jason Ekstrand wrote:
>> We were going out of our way to disable dual-location re-mapping in NIR
>> only to then do the remapping in st_glsl_to_nir.cpp.  Presumably, this
>> was so that double_inputs would be correct for the core state tracker.
>> However, now that we've it to gl_program::DualSlotInputs which is
>> unaffected by NIR lowering, we can let NIR lower things for us.  The one
>> tricky bit here is that we have to remap the inputs_read bitfield back
>> to the single-slot convention for the gallium state tracker to use.
>>
>> Since radeonsi is the only NIR-capable gallium driver that also supports
>> GL_ARB_vertex_attrib_64bit, we only have to worry about radeonsi when
>> making core gallium state tracker changes.
>> ---
>> .../nir/nir_lower_io_arrays_to_elements.c     |  5 +--
>> src/gallium/drivers/radeonsi/si_get.c         |  1 +
>> src/mesa/state_tracker/st_glsl_to_nir.cpp     | 45 +++++++++----------
>> 3 files changed, 22 insertions(+), 29 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir_lower_io_arrays_to_elements.c 
>> b/src/compiler/nir/nir_lower_io_arrays_to_elements.c
>> index 16f6233f614..af33d153ea5 100644
>> --- a/src/compiler/nir/nir_lower_io_arrays_to_elements.c
>> +++ b/src/compiler/nir/nir_lower_io_arrays_to_elements.c
>> @@ -36,9 +36,6 @@ static unsigned
>> get_io_offset(nir_builder *b, nir_deref_instr *deref, nir_variable *var,
>>        unsigned *element_index, nir_ssa_def **vertex_index)
>> {
>> -   bool vs_in = (b->shader->info.stage == MESA_SHADER_VERTEX) &&
>> -                (var->data.mode == nir_var_shader_in);
>> -
>> nir_deref_path path;
>> nir_deref_path_init(&path, deref, NULL);
>>
>> @@ -60,7 +57,7 @@ get_io_offset(nir_builder *b, nir_deref_instr *deref, 
>> nir_variable *var,
>>
>>   assert(c);     /* must not be indirect dereference */
>>
>> -         unsigned size = glsl_count_attribute_slots((*p)->type, vs_in);
>> +         unsigned size = glsl_count_attribute_slots((*p)->type, false);
>>   offset += size * c->u32[0];
>>
>>   unsigned num_elements = glsl_type_is_array((*p)->type) ?
>> diff --git a/src/gallium/drivers/radeonsi/si_get.c 
>> b/src/gallium/drivers/radeonsi/si_get.c
>> index 90f62edf470..bc3d559861f 100644
>> --- a/src/gallium/drivers/radeonsi/si_get.c
>> +++ b/src/gallium/drivers/radeonsi/si_get.c
>> @@ -497,6 +497,7 @@ static const struct nir_shader_compiler_options 
>> nir_options = {
>> .lower_extract_word = true,
>> .max_unroll_iterations = 32,
>> .native_integers = true,
>> +	.vs_inputs_dual_locations = true,
>> };
>>
>> static const void *
>> diff --git a/src/mesa/state_tracker/st_glsl_to_nir.cpp 
>> b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> index 0ee9bd9fef1..d0ec410ec69 100644
>> --- a/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> +++ b/src/mesa/state_tracker/st_glsl_to_nir.cpp
>> @@ -32,6 +32,7 @@
>> #include "program/prog_parameter.h"
>> #include "program/ir_to_mesa.h"
>> #include "main/mtypes.h"
>> +#include "main/imports.h"
>> #include "main/errors.h"
>> #include "main/shaderapi.h"
>> #include "main/uniforms.h"
>> @@ -83,33 +84,18 @@ st_nir_fixup_varying_slots(struct st_context *st, 
>> struct exec_list *var_list)
>> static void
>> st_nir_assign_vs_in_locations(struct gl_program *prog, nir_shader *nir)
>> {
>> -   unsigned attr, num_inputs = 0;
>> -   unsigned input_to_index[VERT_ATTRIB_MAX] = {0};
>> -
>> -   /* TODO de-duplicate w/ similar code in st_translate_vertex_program()? */
>> -   for (attr = 0; attr < VERT_ATTRIB_MAX; attr++) {
>> -      if ((prog->info.inputs_read & BITFIELD64_BIT(attr)) != 0) {
>> -         input_to_index[attr] = num_inputs;
>> -         num_inputs++;
>> -         if ((prog->DualSlotInputs & BITFIELD64_BIT(attr)) != 0) {
>> -            /* add placeholder for second part of a double attribute */
>> -            num_inputs++;
>> -         }
>> -      } else {
>> -         input_to_index[attr] = ~0;
>> -      }
>> -   }
>> -
>> -   /* bit of a hack, mirroring st_translate_vertex_program */
>> -   input_to_index[VERT_ATTRIB_EDGEFLAG] = num_inputs;
>> -
>> nir->num_inputs = 0;
>> nir_foreach_variable_safe(var, &nir->inputs) {
>> -      attr = var->data.location;
>> -      assert(attr < ARRAY_SIZE(input_to_index));
>> -
>> -      if (input_to_index[attr] != ~0u) {
>> -         var->data.driver_location = input_to_index[attr];
>> +      /* NIR already assigns dual-slot inputs to two locations so all we have
>> +       * to do is compact everything down.
>> +       */
>> +      if (var->data.location == VERT_ATTRIB_EDGEFLAG) {
>> +         /* bit of a hack, mirroring st_translate_vertex_program */
>> +         var->data.driver_location = _mesa_bitcount_64(nir->info.inputs_read);
>> +      } else if (nir->info.inputs_read & BITFIELD64_BIT(var->data.location)) {
>> +         var->data.driver_location =
>> +            _mesa_bitcount_64(nir->info.inputs_read &
>> +                              BITFIELD64_MASK(var->data.location));
>
> I'm probably missing something obvious but wont this always set
> driver_location to 1?

That's BITSET_MASK, not BITSET_BIT

>
>>   nir->num_inputs++;
>> } else {
>>   /* Move unused input variables to the globals list (with no
>> @@ -743,6 +729,15 @@ st_link_nir(struct gl_context *ctx,
>>
>> nir_shader_gather_info(nir, nir_shader_get_entrypoint(nir));
>> shader->Program->info = nir->info;
>> +      if (i == MESA_SHADER_VERTEX) {
>> +         /* NIR expands dual-slot inputs out to two locations.  We need to
>> +          * compact things back down GL-style single-slot inputs to avoid
>> +          * confusing the state tracker.
>> +          */
>> +         shader->Program->info.inputs_read =
>> +            nir_get_single_slot_attribs_mask(nir->info.inputs_read,
>> +                                             shader->Program->DualSlotInputs);
>> +      }
>>
>> if (prev != -1) {
>>   struct gl_program *prev_shader =