[Mesa-dev] [PATCH 1/2] draw: fix undefined input handling some more...
Roland Scheidegger
sroland at vmware.com
Fri Nov 4 00:46:16 UTC 2016
Am 03.11.2016 um 18:44 schrieb Jose Fonseca:
> Patch 1 looks good to me. Yes, not using compilation key is a recipe
> for problems.
Indeed. Thankfully no longer used in generate_fetch(). There's still
some state used not from the key, but that's just information belonging
the the shader itself, so should be safe (famous last words...).
>
> Regarding patch 2, I need a bit more time to review. I'd also like the
> new lp_build_umul... function to be added to lp_bld_arit: there is a
> single caller note, but that might change in the future, so might as
> well place it in its natural place.
Fair enough I'll move it. Might as well hook it up then to shader umul
(and imul while I'm at it)... (I'm kinda hoping llvm will fix this soon
- so far actually quite a few people subscribed to the bug but no
comments...).
Roland
>
> José
>
>
>
>
> On Thu, Nov 3, 2016 at 10:48 AM +0000, "Nicolai Hähnle"
> <nhaehnle at gmail.com <mailto:nhaehnle at gmail.com>> wrote:
>
> 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);
>>
>>
More information about the mesa-dev
mailing list