[Mesa-dev] [PATCH 6/9] i965/vec4: Use NIR remapping for VS attributes

[Jason Ekstrand]

On May 5, 2017 4:30:31 PM Kenneth Graunke <kenneth at whitecape.org> wrote:

> On Friday, May 5, 2017 12:31:40 PM PDT Jason Ekstrand wrote:
>> On Fri, May 5, 2017 at 12:38 AM, Alejandro Piñeiro <apinheiro at igalia.com>
>> wrote:
>>
>> > On 05/05/17 04:11, Jason Ekstrand wrote:
>> > > We have to pass inputs_read through from prog_data because we may add an
>> > > edge flag on old platforms.
>> >
>> > Well, the previous code was using nir->info->inputs_read. So perhaps
>> > this explanation should explicitly point that prog_data->inputs_read and
>> > nir->info->inputs_read they are not the same at that point (perhaps2,
>> > and why?)
>> >
>>
>> Sure.  How about:
>>
>> We also change nir_lower_vs_inputs to take an explicit inputs_read bitmask
>> and pass in the inputs_read from prog_data instead from pulling it out of
>> NIR.  This is because the version in prog_data may get EDGEFLAG added to it
>> on some old platforms.
>
> Why don't you just make them the same?  We're operating on a clone of
> the nir_shader anyway, so we can whack nir->info.inputs_read however we
> want.  Then we can just copy it to the prog_data structure, and you
> won't accidentally read the wrong one and screw up.

Because we don't clone the shader_info in nir_clone.  :-(  I've been 
meaning to make nir_shader::info not a pointer.  I guess i should just go 
do that.

>> > Another nitpick and one question below.
>> >
>> > > ---
>> > >  src/intel/compiler/brw_nir.c               | 12 +++----
>> > >  src/intel/compiler/brw_nir.h               |  2 +-
>> > >  src/intel/compiler/brw_vec4.cpp            | 54
>> > +++++++++++-------------------
>> > >  src/intel/compiler/brw_vec4_nir.cpp        | 49
>> > ++-------------------------
>> > >  src/intel/compiler/brw_vec4_visitor.cpp    |  7 ++--
>> > >  src/intel/compiler/brw_vec4_vs_visitor.cpp | 31 ++---------------
>> > >  6 files changed, 33 insertions(+), 122 deletions(-)
>> > >
>> > > diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
>> > > index 556782e..5ca532f 100644
>> > > --- a/src/intel/compiler/brw_nir.c
>> > > +++ b/src/intel/compiler/brw_nir.c
>> > > @@ -230,7 +230,7 @@ remap_patch_urb_offsets(nir_block *block,
>> > nir_builder *b,
>> > >
>> > >  void
>> > >  brw_nir_lower_vs_inputs(nir_shader *nir,
>> > > -                        bool is_scalar,
>> > > +                        uint64_t inputs_read,
>> > >                          bool use_legacy_snorm_formula,
>> > >                          const uint8_t *vs_attrib_wa_flags)
>> > >  {
>> > > @@ -253,11 +253,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
>> > >     brw_nir_apply_attribute_workarounds(nir, use_legacy_snorm_formula,
>> > >                                         vs_attrib_wa_flags);
>> > >
>> > > -   /* The last step is to remap VERT_ATTRIB_* to actual registers and
>> > we only
>> > > -    * do that for scalar shaders at the moment.
>> > > -    */
>> > > -   if (!is_scalar)
>> > > -      return;
>> > > +   /* The last step is to remap VERT_ATTRIB_* to actual registers */
>> > >
>> > >     const bool has_svgs =
>> > >        nir->info->system_values_read &
>> > > @@ -266,7 +262,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
>> > >         BITFIELD64_BIT(SYSTEM_VALUE_VERTEX_ID_ZERO_BASE) |
>> > >         BITFIELD64_BIT(SYSTEM_VALUE_INSTANCE_ID));
>> > >
>> > > -   const unsigned num_inputs = _mesa_bitcount_64(nir->info->
>> > inputs_read);
>> > > +   const unsigned num_inputs = _mesa_bitcount_64(inputs_read);
>> > >
>> > >     nir_foreach_function(function, nir) {
>> > >        if (!function->impl)
>> > > @@ -340,7 +336,7 @@ brw_nir_lower_vs_inputs(nir_shader *nir,
>> > >                  * before it and counting the bits.
>> > >                  */
>> > >                 int attr = nir_intrinsic_base(intrin);
>> > > -               int slot = _mesa_bitcount_64(nir->info->inputs_read &
>> > > +               int slot = _mesa_bitcount_64(inputs_read &
>> > >                                              BITFIELD64_MASK(attr));
>> > >                 nir_intrinsic_set_base(intrin, slot);
>> > >                 break;
>> > > diff --git a/src/intel/compiler/brw_nir.h b/src/intel/compiler/brw_nir.h
>> > > index b96072c..3bba68d 100644
>> > > --- a/src/intel/compiler/brw_nir.h
>> > > +++ b/src/intel/compiler/brw_nir.h
>> > > @@ -98,7 +98,7 @@ nir_shader *brw_preprocess_nir(const struct
>> > brw_compiler *compiler,
>> > >  bool brw_nir_lower_intrinsics(nir_shader *nir,
>> > >                                struct brw_stage_prog_data *prog_data);
>> > >  void brw_nir_lower_vs_inputs(nir_shader *nir,
>> > > -                             bool is_scalar,
>> > > +                             uint64_t inputs_read,
>> > >                               bool use_legacy_snorm_formula,
>> > >                               const uint8_t *vs_attrib_wa_flags);
>> > >  void brw_nir_lower_vue_inputs(nir_shader *nir, bool is_scalar,
>
> With that suggestion, you can drop all of the above changes.