On Tue, Nov 3, 2015 at 10:58 AM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > On 3 November 2015 at 18:20, Matt Turner <matts...@gmail.com> wrote: >> On Tue, Nov 3, 2015 at 8:09 AM, Emil Velikov <emil.l.veli...@gmail.com> >> wrote: >>> On 3 November 2015 at 00:29, Matt Turner <matts...@gmail.com> wrote: >>> >>>> @@ -387,7 +342,9 @@ vec4_visitor::opt_vector_float() >>>> >>>> remaining_channels &= ~inst->dst.writemask; >>>> if (remaining_channels == 0) { >>>> - vec4_instruction *mov = MOV(inst->dst, imm); >>>> + unsigned vf; >>>> + memcpy(&vf, imm, sizeof(vf)); >>>> + vec4_instruction *mov = MOV(inst->dst, brw_imm_vf(vf)); >>> You can drop the temp variable + memcpy call and use >>> brw_imm_vf4(imm[x],....) >> >> Ah, yes, I can. And it even generates identical code. >> >> Unfortunately, gcc isn't smart enough to understand that imm[] is not >> uninitialized. Wonder if it just doesn't warn about src arguments to >> memcpy()? >> > Actually it seems like a genuine warning.
Coverity warns about the code I committed in much the same way -- it thinks an element of imm[] might be uninitialized. > > if (inst->opcode != BRW_OPCODE_MOV || > inst->dst.writemask == WRITEMASK_XYZW || > inst->src[0].file != IMM) > continue; > > int vf = brw_float_to_vf(inst->src[0].fixed_hw_reg.dw1.f); > if (vf == -1) > continue; > > if ((inst->dst.writemask & WRITEMASK_X) != 0) > imm[0] = vf; > if ((inst->dst.writemask & WRITEMASK_Y) != 0) > imm[1] = vf; > if ((inst->dst.writemask & WRITEMASK_Z) != 0) > imm[2] = vf; > if ((inst->dst.writemask & WRITEMASK_W) != 0) > imm[3] = vf; > > imm_inst[inst_count++] = inst; > > remaining_channels &= ~inst->dst.writemask; > if (remaining_channels == 0) { > vec4_instruction *mov = MOV(inst->dst, imm); > mov->dst.type = BRW_REGISTER_TYPE_F; > mov->dst.writemask = WRITEMASK_XYZW; > > > From the above, if all the X Y Z and W writemasks are set, we just > jump to the next instruction. Thus, at least one of the imm values is > uninitialised. We might have found ourselves a bug :-) I can't see it, but that might be because I can't stop thinking like I was when I wrote the code. Yeah, we skip any instruction that has a full writemask -- because if it has a full writemask, we can just load the immediate as-is and we don't need to turn it into 4x restricted floats. But that doesn't seem to be related to the warning. The code seems fine to me. Here's what I see: We start with remaining_channels = WRITEMASK_XYZW. We initialize an element of imm[] for each enabled bit of inst->dst.writemask, and then we disable those bits from remaining_channels. When remaining_channels is zero (should be if and only if all elements of imm[] are initialized), we read imm[]. Do you see anything wrong or anything I've missed? FWIW, Clang does not warn about this and doesn't warn if I remove the useless initializations of remaining_channels = 0 and inst_count = 0. I think this is gcc and Coverity just being stupid. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev