[Mesa-dev] [PATCH 23/24] i965/vec4: Replace src_reg(imm) constructors with brw_imm_*().

[Matt Turner]

On Tue, Nov 3, 2015 at 10:58 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 3 November 2015 at 18:20, Matt Turner <mattst88 at gmail.com> wrote:
>> On Tue, Nov 3, 2015 at 8:09 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>> On 3 November 2015 at 00:29, Matt Turner <mattst88 at 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.