[Mesa-dev] [PATCH v3] compiler: Mark when input/ouput attribute at VS uses 16-bit (v2)

[Chema Casanova]

El 02/11/17 a las 19:25, Jason Ekstrand escribió:
> On Thu, Nov 2, 2017 at 11:17 AM, Chema Casanova <jmcasanova at igalia.com
> <mailto:jmcasanova at igalia.com>> wrote:
>
>
>
>     El 01/11/17 a las 22:07, Jason Ekstrand escribió:
>     > On Tue, Oct 17, 2017 at 10:05 AM, Jose Maria Casanova Crespo
>     > <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>
>     <mailto:jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>>> wrote:
>     >
>     >     New shader attribute to mark when a location has 16-bit
>     >     value. This patch includes support on mesa glsl and nir.
>     >
>     >     v2: Remove use of is_half_slot as is a duplicate of is_16bit
>     >         (Topi Pohjolainen)
>     >     ---
>     >      src/compiler/glsl_types.h          | 15 +++++++++++++++
>     >      src/compiler/nir/nir_gather_info.c | 21 ++++++++++++++-------
>     >      src/compiler/shader_info.h         |  2 ++
>     >      3 files changed, 31 insertions(+), 7 deletions(-)
>     >
>     >     diff --git a/src/compiler/glsl_types.h
>     b/src/compiler/glsl_types.h
>     >     index 32399df351..e35e8d8f88 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,
>     >     @@ -555,6 +562,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..cce64f9c84 100644
>     >     --- a/src/compiler/nir/nir_gather_info.c
>     >     +++ b/src/compiler/nir/nir_gather_info.c
>     >     @@ -212,14 +212,20 @@ 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_get_bit_size(glsl_without_array(var->type)) == 16) {
>     >     +               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 +318,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/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;
>     >
>     >
>     > Given that we're flagging this for 16-bit integers, I don't think
>     > "half" is really appropriate.  How about just 16bit_inputs_read?
>
>     I thought about that, but we can not do that. As the C restriction of
>     variable names starting with alphabet or underscore. As the logic was
>     the same as for double I didn't want to go for a inputs_read_16bits. I
>     didn't come up with a better name. :(
>
>
> Ugh. :-(  You could always do _16bit_inputs_read.  Or
> inputs_read_16bit.  Really the double one will probably have to get
> renamed too since we have int64.

So let's go with inputs_read_16bit and we we'll rename in the future the
double ones for inputs_read_64bit

Chema