Re: [Mesa-dev] [PATCH v3 25/43] compiler: Mark when input/ouput attribute at VS uses 16-bit
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 +
Re: [Mesa-dev] [PATCH v3 25/43] compiler: Mark when input/ouput attribute at VS uses 16-bit
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().
> + {
> + 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.
> + 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
> ---
