Based on my (weak) understanding things in this part of the code, I
think this is ok. There are a couple minor nits below. With those
addressed and unless Jason has (time to give) some objections, this patch is
Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
On 11/28/2017 02:06 AM, Samuel Iglesias Gonsálvez wrote:
> This patch is still unreviewed.
>
> Sam
>
> On Wed, 2017-11-15 at 07:39 +0100, Samuel Iglesias Gonsálvez wrote:
>> We can write to the same output but in different components, like
>> in this example:
>>
>> layout(location = 0, component = 0) out ivec2 dEQP_FragColor_0;
>> layout(location = 0, component = 2) out ivec2 dEQP_FragColor_1;
>>
>> Therefore, they are not two different outputs but only one.
>>
>> Fixes:
>>
>> dEQP-VK.glsl.440.linkage.varying.component.frag_out.*
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
>> ---
>> src/compiler/shader_enums.h | 1 +
>> src/intel/vulkan/anv_pipeline.c | 46 +++++++++++++++++++++++++++--
>> ------------
>> 2 files changed, 32 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/compiler/shader_enums.h
>> b/src/compiler/shader_enums.h
>> index 9d229d4199e..90729dbfd96 100644
>> --- a/src/compiler/shader_enums.h
>> +++ b/src/compiler/shader_enums.h
>> @@ -603,6 +603,7 @@ typedef enum
>> FRAG_RESULT_DATA5,
>> FRAG_RESULT_DATA6,
>> FRAG_RESULT_DATA7,
>> + FRAG_RESULT_MAX, /**< Number of fragment program results */
Does this add warnings about enum values not handled in switch statements?
>> } gl_frag_result;
>>
>> const char *gl_frag_result_name(gl_frag_result result);
>> diff --git a/src/intel/vulkan/anv_pipeline.c
>> b/src/intel/vulkan/anv_pipeline.c
>> index 907b24a758d..92cfc2898f5 100644
>> --- a/src/intel/vulkan/anv_pipeline.c
>> +++ b/src/intel/vulkan/anv_pipeline.c
>> @@ -870,30 +870,28 @@ anv_pipeline_compile_fs(struct anv_pipeline
>> *pipeline,
>> }
>>
>> unsigned num_rts = 0;
>> - struct anv_pipeline_binding rt_bindings[8];
>> + const int max_rt = FRAG_RESULT_MAX - FRAG_RESULT_DATA0;
>> + struct anv_pipeline_binding rt_bindings[max_rt];
>> nir_function_impl *impl = nir_shader_get_entrypoint(nir);
>> + int rt_to_bindings[max_rt];
>> + memset(rt_to_bindings, -1, sizeof(int) * max_rt);
Isn't this just sizeof(rt_to_bindings)?
>> +
>> + /* Set new, compacted, location */
>> nir_foreach_variable_safe(var, &nir->outputs) {
>> if (var->data.location < FRAG_RESULT_DATA0)
>> continue;
>>
>> unsigned rt = var->data.location - FRAG_RESULT_DATA0;
>> - if (rt >= key.nr_color_regions) {
>> - /* Out-of-bounds, throw it away */
>> - var->data.mode = nir_var_local;
>> - exec_node_remove(&var->node);
>> - exec_list_push_tail(&impl->locals, &var->node);
>> + if (rt_to_bindings[rt] != -1 || rt >= key.nr_color_regions)
>> continue;
>> - }
>> -
>> - /* Give it a new, compacted, location */
>> - var->data.location = FRAG_RESULT_DATA0 + num_rts;
>> -
>> - unsigned array_len =
>> + const unsigned array_len =
>> glsl_type_is_array(var->type) ? glsl_get_length(var-
>>> type) : 1;
>> - assert(num_rts + array_len <= 8);
>> + assert(num_rts + array_len <= max_rt);
>> +
>> + rt_to_bindings[rt] = num_rts;
>>
>> for (unsigned i = 0; i < array_len; i++) {
>> - rt_bindings[num_rts + i] = (struct anv_pipeline_binding)
>> {
>> + rt_bindings[rt_to_bindings[rt] + i] = (struct
>> anv_pipeline_binding) {
>> .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
>> .binding = 0,
>> .index = rt + i,
>> @@ -903,6 +901,24 @@ anv_pipeline_compile_fs(struct anv_pipeline
>> *pipeline,
>> num_rts += array_len;
>> }
>>
>> + nir_foreach_variable_safe(var, &nir->outputs) {
>> + if (var->data.location < FRAG_RESULT_DATA0)
>> + continue;
>> +
>> + unsigned rt = var->data.location - FRAG_RESULT_DATA0;
const
>> + if (rt >= key.nr_color_regions) {
>> + /* Out-of-bounds, throw it away */
>> + var->data.mode = nir_var_local;
>> + exec_node_remove(&var->node);
>> + exec_list_push_tail(&impl->locals, &var->node);
>> + continue;
>> + }
>> +
>> + /* Give it the new location */
>> + assert(rt_to_bindings[rt] != -1);
>> + var->data.location = rt_to_bindings[rt] +
>> FRAG_RESULT_DATA0;
>> + }
>> +
>> if (num_rts == 0) {
>> /* If we have no render targets, we need a null render
>> target */
>> rt_bindings[0] = (struct anv_pipeline_binding) {
>> @@ -913,7 +929,7 @@ anv_pipeline_compile_fs(struct anv_pipeline
>> *pipeline,
>> num_rts = 1;
>> }
>>
>> - assert(num_rts <= 8);
>> + assert(num_rts <= max_rt);
>> map.surface_to_descriptor -= num_rts;
>> map.surface_count += num_rts;
>> assert(map.surface_count <= 256);
> _______________________________________________
> 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
