On 15/10/17 12:00, Pohjolainen, Topi wrote:
> On Thu, Oct 12, 2017 at 08:38:14PM +0200, Jose Maria Casanova Crespo wrote:
>> New shader attribute to mark when a location has 16-bit
>> value. This patch includes support on mesa glsl and nir.
>> ---
>>  src/compiler/glsl_types.h          | 24 ++++++++++++++++++++++++
>>  src/compiler/nir/nir_gather_info.c | 23 ++++++++++++++++-------
>>  src/compiler/nir_types.cpp         |  6 ++++++
>>  src/compiler/nir_types.h           |  1 +
>>  src/compiler/shader_info.h         |  2 ++
>>  5 files changed, 49 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h
>> index 32399df351..d05e612e66 100644
>> --- a/src/compiler/glsl_types.h
>> +++ b/src/compiler/glsl_types.h
>> @@ -93,6 +93,13 @@ static inline bool glsl_base_type_is_integer(enum 
>> glsl_base_type type)
>>            type == GLSL_TYPE_IMAGE;
>>  }
>>  
>> +static inline bool glsl_base_type_is_16bit(enum glsl_base_type type)
>> +{
>> +   return type == GLSL_TYPE_FLOAT16 ||
>> +          type == GLSL_TYPE_UINT16 ||
>> +          type == GLSL_TYPE_INT16;
>> +}
>> +
>>  enum glsl_sampler_dim {
>>     GLSL_SAMPLER_DIM_1D = 0,
>>     GLSL_SAMPLER_DIM_2D,
>> @@ -546,6 +553,15 @@ struct glsl_type {
>>        return is_64bit() && vector_elements > 2;
>>     }
>>  
>> +
>> +   /**
>> +    * Query whether a 16-bit type takes half slots.
>> +    */
>> +   bool is_half_slot() const
> 
> I haven't checked later patches but here at least I'm wondering why we need
> two functionally identical helpers with different names, i.e., is_half_slot()
> and is_16bit().

It is true that at this moment, any use of is_half_slot could be
directly changed for is_16bit.

So removing is_half_slot could simplify the understanding of the code.
Because at the end the idea behind having two names was simply to use
the concept of half_slots when tracking the location input attributes at
the VS with 16-bit in a similar way that it was done for 64-bits for
dual slots (64bits & (vec3 || vec4)) .

After thinking about it it would also clearer maintain the is_16bit as
helper for future uses. But in the particular case of checking half
slots we could just use:

(glsl_get_bit_size(glsl_without_array(var->type)) == 16)


In this case what we really matters is that we have 16-bit values so we
need to unshuffle them, independently that they use half of an slot that
is the case of 16-bits values.

>> +   {
>> +      return is_16bit();
>> +   }
>> +
>>     /**
>>      * Query whether or not a type is 64-bit
>>      */
>> @@ -555,6 +571,14 @@ struct glsl_type {
>>     }
>>  
>>     /**
>> +    * Query whether or not a type is 16-bit
>> +    */
>> +   bool is_16bit() const
>> +   {
>> +      return glsl_base_type_is_16bit(base_type);
>> +   }
>> +
>> +   /**
>>      * Query whether or not a type is a non-array boolean type
>>      */
>>     bool is_boolean() const
>> diff --git a/src/compiler/nir/nir_gather_info.c 
>> b/src/compiler/nir/nir_gather_info.c
>> index ac87bec46c..c7f8ff29cb 100644
>> --- a/src/compiler/nir/nir_gather_info.c
>> +++ b/src/compiler/nir/nir_gather_info.c
>> @@ -212,14 +212,22 @@ gather_intrinsic_info(nir_intrinsic_instr *instr, 
>> nir_shader *shader)
>>           if (!try_mask_partial_io(shader, instr->variables[0]))
>>              mark_whole_variable(shader, var);
>>  
>> -         /* We need to track which input_reads bits correspond to a
>> -          * dvec3/dvec4 input attribute */
>> +         /* We need to track which input_reads bits correspond to
>> +          * dvec3/dvec4 or 16-bit  input attributes */
>>           if (shader->stage == MESA_SHADER_VERTEX &&
>> -             var->data.mode == nir_var_shader_in &&
>> -             glsl_type_is_dual_slot(glsl_without_array(var->type))) {
>> -            for (uint i = 0; i < glsl_count_attribute_slots(var->type, 
>> false); i++) {
>> -               int idx = var->data.location + i;
>> -               shader->info.double_inputs_read |= BITFIELD64_BIT(idx);
>> +             var->data.mode == nir_var_shader_in) {
>> +            if (glsl_type_is_dual_slot(glsl_without_array(var->type))) {
>> +               for (uint i = 0; i < glsl_count_attribute_slots(var->type, 
>> false); i++) {
>> +                  int idx = var->data.location + i;
>> +                  shader->info.double_inputs_read |= BITFIELD64_BIT(idx);
>> +               }
>> +            } else {
>> +               if (glsl_type_is_half_slot(glsl_without_array(var->type))) {
> 
> This could be:
> 
>                } else if 
> (glsl_type_is_half_slot(glsl_without_array(var->type))) {
> 
> allowing us to reduce indentation in the block.

Also changing this with the change proposed before,

} else if (glsl_get_bit_size(glsl_without_array(var->type)) == 16) {

I'm sending an v2 of this patch with these changes.

>> +                  for (uint i = 0; i < 
>> glsl_count_attribute_slots(var->type, false); i++) {
>> +                     int idx = var->data.location + i;
>> +                     shader->info.half_inputs_read |= BITFIELD64_BIT(idx);
>> +                  }
>> +               }
>>              }
>>           }
>>        }
>> @@ -312,6 +320,7 @@ nir_shader_gather_info(nir_shader *shader, 
>> nir_function_impl *entrypoint)
>>     shader->info.outputs_written = 0;
>>     shader->info.outputs_read = 0;
>>     shader->info.double_inputs_read = 0;
>> +   shader->info.half_inputs_read = 0;
>>     shader->info.patch_inputs_read = 0;
>>     shader->info.patch_outputs_written = 0;
>>     shader->info.system_values_read = 0;
>> diff --git a/src/compiler/nir_types.cpp b/src/compiler/nir_types.cpp
>> index ae594eb97f..cb95f54f78 100644
>> --- a/src/compiler/nir_types.cpp
>> +++ b/src/compiler/nir_types.cpp
>> @@ -236,6 +236,12 @@ glsl_type_is_dual_slot(const struct glsl_type *type)
>>  }
>>  
>>  bool
>> +glsl_type_is_half_slot(const struct glsl_type *type)
>> +{
>> +   return type->is_half_slot();
>> +}
>> +
>> +bool
>>  glsl_type_is_numeric(const struct glsl_type *type)
>>  {
>>     return type->is_numeric();
>> diff --git a/src/compiler/nir_types.h b/src/compiler/nir_types.h
>> index 739109e32f..444b707dbd 100644
>> --- a/src/compiler/nir_types.h
>> +++ b/src/compiler/nir_types.h
>> @@ -123,6 +123,7 @@ bool glsl_type_is_struct(const struct glsl_type *type);
>>  bool glsl_type_is_sampler(const struct glsl_type *type);
>>  bool glsl_type_is_image(const struct glsl_type *type);
>>  bool glsl_type_is_dual_slot(const struct glsl_type *type);
>> +bool glsl_type_is_half_slot(const struct glsl_type *type);
>>  bool glsl_type_is_numeric(const struct glsl_type *type);
>>  bool glsl_type_is_boolean(const struct glsl_type *type);
>>  bool glsl_sampler_type_is_shadow(const struct glsl_type *type);
>> diff --git a/src/compiler/shader_info.h b/src/compiler/shader_info.h
>> index 38413940d6..98111fa1e0 100644
>> --- a/src/compiler/shader_info.h
>> +++ b/src/compiler/shader_info.h
>> @@ -55,6 +55,8 @@ typedef struct shader_info {
>>     uint64_t inputs_read;
>>     /* Which inputs are actually read and are double */
>>     uint64_t double_inputs_read;
>> +   /* Which inputs are actually read and are half */
>> +   uint64_t half_inputs_read;
>>     /* Which outputs are actually written */
>>     uint64_t outputs_written;
>>     /* Which outputs are actually read */
>> -- 
>> 2.13.6
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to