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
