[Mesa-dev] [PATCH] ac/nir: silence maybe-uninitialized warnings

[Nicolai Hähnle]

On 28.08.2017 10:30, Nicolai Hähnle wrote:
> On 27.08.2017 23:43, Grazvydas Ignotas wrote:
>> These are likely false positives, but are also annoying because they
>> show up on every "make install", which causes ac_nir_to_llvm to be
>> rebuilt here. Initializing those variables to NULL should be harmless
>> even when unnecessary.
>>
>> Signed-off-by: Grazvydas Ignotas <notasas at gmail.com>
>> ---
>>   src/amd/common/ac_nir_to_llvm.c | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/amd/common/ac_nir_to_llvm.c 
>> b/src/amd/common/ac_nir_to_llvm.c
>> index 664d83b..823ea42 100644
>> --- a/src/amd/common/ac_nir_to_llvm.c
>> +++ b/src/amd/common/ac_nir_to_llvm.c
>> @@ -1332,11 +1332,11 @@ static LLVMValueRef emit_i2b(struct 
>> ac_llvm_context *ctx,
>>   static LLVMValueRef emit_f2f16(struct nir_to_llvm_context *ctx,
>>                      LLVMValueRef src0)
>>   {
>>       LLVMValueRef result;
>> -    LLVMValueRef cond;
>> +    LLVMValueRef cond = NULL;
>>       src0 = to_float(&ctx->ac, src0);
>>       result = LLVMBuildFPTrunc(ctx->builder, src0, ctx->f16, "");
>>       if (ctx->options->chip_class >= VI) {
>> @@ -2052,11 +2052,11 @@ static LLVMValueRef 
>> radv_lower_gather4_integer(struct ac_llvm_context *ctx,
>>                              const nir_tex_instr *instr)
>>   {
>>       enum glsl_base_type stype = 
>> glsl_get_sampler_result_type(instr->texture->var->type);
>>       LLVMValueRef coord = args->addr;
>>       LLVMValueRef half_texel[2];
>> -    LLVMValueRef compare_cube_wa;
>> +    LLVMValueRef compare_cube_wa = NULL;
>>       LLVMValueRef result;
>>       int c;
>>       unsigned coord_vgpr_index = (unsigned)args->offset + 
>> (unsigned)args->compare;
>>       //TODO Rect
>> @@ -2778,11 +2778,12 @@ load_tcs_input(struct nir_to_llvm_context *ctx,
>>   static LLVMValueRef
>>   load_tcs_output(struct nir_to_llvm_context *ctx,
>>              nir_intrinsic_instr *instr)
>>   {
>> -    LLVMValueRef dw_addr, stride;
>> +    LLVMValueRef dw_addr;
>> +    LLVMValueRef stride = NULL;
>>       LLVMValueRef value[4], result;
>>       LLVMValueRef vertex_index = NULL;
>>       LLVMValueRef indir_index = NULL;
>>       unsigned const_index = 0;
>>       unsigned param;
>> @@ -2817,11 +2818,12 @@ static void
>>   store_tcs_output(struct nir_to_llvm_context *ctx,
>>            nir_intrinsic_instr *instr,
>>            LLVMValueRef src,
>>            unsigned writemask)
>>   {
>> -    LLVMValueRef stride, dw_addr;
>> +    LLVMValueRef dw_addr;
>> +    LLVMValueRef stride = NULL;
>>       LLVMValueRef buf_addr = NULL;
>>       LLVMValueRef vertex_index = NULL;
>>       LLVMValueRef indir_index = NULL;
>>       unsigned const_index = 0;
>>       unsigned param;
>> @@ -3817,12 +3819,13 @@ static LLVMValueRef visit_interp(struct 
>> nir_to_llvm_context *ctx,
>>   {
>>       LLVMValueRef result[2];
>>       LLVMValueRef interp_param, attr_number;
>>       unsigned location;
>>       unsigned chan;
>> -    LLVMValueRef src_c0, src_c1;
>> -    LLVMValueRef src0;
>> +    LLVMValueRef src_c0 = NULL;
>> +    LLVMValueRef src_c1 = NULL;
>> +    LLVMValueRef src0 = NULL;
> 
> I think this one actually involves a real bug. Specifically, I suspect 
> that the
> 
>      if (location == INTERP_SAMPLE || location == INTERP_CENTER) {
> 
> below should really be
> 
>      if (location == INTERP_SAMPLE || location == INTERP_OFFSET) {
> 
> because src_c0/c1 are set, and the enclosed logic makes sense, only in 
> the nir_intrinsic_interp_var_at_{sample,offset} cases.

Never mind, that part is misleading. location refers to the base 
location, not the final location of the sample, and it can never be 
INTERP_SAMPLE.

It'd make sense to to change it to

     if (location == INTERP_CENTER) [

Whether or not you decide to include that in the patch, it's

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


> 
> Apart from that, the patch looks good to me.
> 
> Cheers,
> Nicolai
> 
> 
>>       int input_index = instr->variables[0]->var->data.location - 
>> VARYING_SLOT_VAR0;
>>       switch (instr->intrinsic) {
>>       case nir_intrinsic_interp_var_at_centroid:
>>           location = INTERP_CENTROID;
>>           break;
>>
> 
> 


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.