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

[Emil Velikov]

On 25 November 2015 at 22:48, Matt Turner <mattst88 at gmail.com> wrote:

> I can't see it, but that might be because I can't stop thinking like I
> was when I wrote the code.
>
Understandable, we've all been there.

> 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.
Actually we start with WRITEMASK_NONE (0). Based on your knowledge you
can establish that on the first iteration(?) remaining_channels will
be set to WRITEMASK_XYZW, although the compiler cannot determine that.

> We initialize an
> element of imm[] for each enabled bit of inst->dst.writemask, and then
> we disable those bits from remaining_channels.
If all channels are set we bail out just before that. Thus as we hit
the imm[x] assignments of at least one channel is not set. Be that
left uninitialized or storing data from a previous instruction. I take
that this is by design ?

> When remaining_channels
> is zero (should be if and only if all elements of imm[] are
> initialized), we read imm[].
>
Barring your existing knowledge, remaining_channels can be zero on the
first, second,  N'th iteration, albeit extremely unlikely. Thus
regardless of what data we have in imm[], we'll proceed to use it.

> 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.
I think the whole things is that you know who it will run, whereas the
code makes is a bit ambiguous.

To fix (?) and make things a lot more obvious I'd suggest initializing
(upon declaration) remaining_channels with WRITEMASK_XYZW. If
gcc/coverity/other complains with that in place then it's definitely a
defect on their end.

Out of curiosity: what is the input from gcc devs, do you have the bug
number handy ?

Regards,
Emil