[Mesa-dev] [PATCH 1/2] draw: fix undefined input handling some more...

[Nicolai Hähnle]

On 03.11.2016 02:15, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> Previous fixes were incomplete - some code still iterated through the number
> of elements provided by velem layout instead of the number stored in the key
> (which is the same as the number defined by the vs). And also actually
> accessed the elements from the layout directly instead of those in the key.
> This mismatch could still cause crashes.
> (Besides, it is a very good idea to only use data stored in the key anyway.)

Makes sense to me. It would make sense to pull the check for 
PIPE_FORMAT_NONE from generate_fetch into draw_llvm_generate for 
consistency between the two loops. Either way,

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

> ---
>  src/gallium/auxiliary/draw/draw_llvm.c | 77 ++++++++++++++++++----------------
>  1 file changed, 40 insertions(+), 37 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c b/src/gallium/auxiliary/draw/draw_llvm.c
> index 2f82d9d..d5fc1c2 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
> @@ -1658,10 +1658,10 @@ draw_llvm_generate(struct draw_llvm *llvm, struct draw_llvm_variant *variant,
>     /*
>      * Pre-calculate everything which is constant per shader invocation.
>      */
> -   for (j = 0; j < draw->pt.nr_vertex_elements; ++j) {
> +   for (j = 0; j < key->nr_vertex_elements; ++j) {
>        LLVMValueRef vb_buffer_offset, buffer_size;
>        LLVMValueRef vb_info, vbuffer_ptr;
> -      struct pipe_vertex_element *velem = &draw->pt.vertex_element[j];
> +      struct pipe_vertex_element *velem = &key->vertex_element[j];
>        LLVMValueRef vb_index =
>           lp_build_const_int32(gallivm, velem->vertex_buffer_index);
>        LLVMValueRef bsize = lp_build_const_int32(gallivm,
> @@ -1669,41 +1669,44 @@ draw_llvm_generate(struct draw_llvm *llvm, struct draw_llvm_variant *variant,
>        LLVMValueRef src_offset = lp_build_const_int32(gallivm,
>                                                       velem->src_offset);
>
> -      vbuffer_ptr = LLVMBuildGEP(builder, vbuffers_ptr, &vb_index, 1, "");
> -      vb_info = LLVMBuildGEP(builder, vb_ptr, &vb_index, 1, "");
> -      vb_stride[j] = draw_jit_vbuffer_stride(gallivm, vb_info);
> -      vb_buffer_offset = draw_jit_vbuffer_offset(gallivm, vb_info);
> -      map_ptr[j] = draw_jit_dvbuffer_map(gallivm, vbuffer_ptr);
> -      buffer_size = draw_jit_dvbuffer_size(gallivm, vbuffer_ptr);
> -
> -      ofbit[j] = NULL;
> -      stride_fixed[j] = lp_build_uadd_overflow(gallivm, vb_buffer_offset,
> -                                               src_offset, &ofbit[j]);
> -      buffer_size_adj[j] = lp_build_usub_overflow(gallivm, buffer_size, bsize,
> -                                                   &ofbit[j]);
> -
> -      if (velem->instance_divisor) {
> -         /* Index is equal to the start instance plus the number of current
> -          * instance divided by the divisor. In this case we compute it as:
> -          * index = start_instance + (instance_id  / divisor)
> -          */
> -         LLVMValueRef current_instance;
> -         current_instance = LLVMBuildUDiv(builder, system_values.instance_id,
> -                                          lp_build_const_int32(gallivm,
> -                                                               velem->instance_divisor),
> -                                          "instance_divisor");
> -         instance_index[j] = lp_build_uadd_overflow(gallivm, start_instance,
> -                                                    current_instance, &ofbit[j]);
> -      }
> +      if (velem->src_format != PIPE_FORMAT_NONE) {
> +         vbuffer_ptr = LLVMBuildGEP(builder, vbuffers_ptr, &vb_index, 1, "");
> +         vb_info = LLVMBuildGEP(builder, vb_ptr, &vb_index, 1, "");
> +         vb_stride[j] = draw_jit_vbuffer_stride(gallivm, vb_info);
> +         vb_buffer_offset = draw_jit_vbuffer_offset(gallivm, vb_info);
> +         map_ptr[j] = draw_jit_dvbuffer_map(gallivm, vbuffer_ptr);
> +         buffer_size = draw_jit_dvbuffer_size(gallivm, vbuffer_ptr);
> +
> +         ofbit[j] = NULL;
> +         stride_fixed[j] = lp_build_uadd_overflow(gallivm, vb_buffer_offset,
> +                                                  src_offset, &ofbit[j]);
> +         buffer_size_adj[j] = lp_build_usub_overflow(gallivm, buffer_size, bsize,
> +                                                     &ofbit[j]);
> +
> +         if (velem->instance_divisor) {
> +            /* Index is equal to the start instance plus the number of current
> +             * instance divided by the divisor. In this case we compute it as:
> +             * index = start_instance + (instance_id  / divisor)
> +             */
> +            LLVMValueRef current_instance;
> +            current_instance = LLVMBuildUDiv(builder, system_values.instance_id,
> +                                             lp_build_const_int32(gallivm,
> +                                                                  velem->instance_divisor),
> +                                             "instance_divisor");
> +            instance_index[j] = lp_build_uadd_overflow(gallivm, start_instance,
> +                                                       current_instance, &ofbit[j]);
> +         }
>
> -      if (0) {
> -         lp_build_printf(gallivm, "vbuf index = %u, vb_stride is %u\n",
> -                         vb_index, vb_stride[j]);
> -         lp_build_printf(gallivm, "   vb_buffer_offset = %u, src_offset is %u\n",
> -                         vb_buffer_offset, src_offset);
> -         lp_build_print_value(gallivm, "   blocksize = ", bsize);
> -         lp_build_printf(gallivm, "   instance_id = %u\n", system_values.instance_id);
> -         lp_build_printf(gallivm, "   buffer size = %u\n", buffer_size);
> +         if (0) {
> +            lp_build_printf(gallivm, "vbuf index = %u, vb_stride is %u\n",
> +                            vb_index, vb_stride[j]);
> +            lp_build_printf(gallivm, "   vb_buffer_offset = %u, src_offset is %u\n",
> +                            vb_buffer_offset, src_offset);
> +            lp_build_print_value(gallivm, "   blocksize = ", bsize);
> +            lp_build_printf(gallivm, "   instance_id = %u\n",
> +                            system_values.instance_id);
> +            lp_build_printf(gallivm, "   buffer size = %u\n", buffer_size);
> +         }
>        }
>     }
>
> @@ -1782,7 +1785,7 @@ draw_llvm_generate(struct draw_llvm *llvm, struct draw_llvm_variant *variant,
>        }
>
>        for (j = 0; j < key->nr_vertex_elements; ++j) {
> -         struct pipe_vertex_element *velem = &draw->pt.vertex_element[j];
> +         struct pipe_vertex_element *velem = &key->vertex_element[j];
>           const struct util_format_description *format_desc =
>              util_format_description(velem->src_format);
>
>