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

[Emil Velikov]

On 26 November 2015 at 22:09, Matt Turner <mattst88 at gmail.com> wrote:
> On Thu, Nov 26, 2015 at 9:22 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> 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.
>
> The initialization of remaining_writemask is *dead* -- the first block
> inside the for loop initializes it upon seeing the first instruction.
True, although I was suggesting that this is not apparent to the compiler.

> Initializing it to WRITEMASK_XYZW doesn't fix the warning in any case.
>
Any objections if we either change the it to WRITEMASK_XYZW or just
nuke the initialization all together ? Guessing that you're leaning
towards the latter.

>> Out of curiosity: what is the input from gcc devs, do you have the bug
>> number handy ?
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68548
>
> They think it's a deficiency in gcc.
Nice I'll keep an eye how this progress

Thanks
Emil