On 26 November 2015 at 22:09, Matt Turner <matts...@gmail.com> wrote: > On Thu, Nov 26, 2015 at 9:22 AM, Emil Velikov <emil.l.veli...@gmail.com> > wrote: >> On 25 November 2015 at 22:48, Matt Turner <matts...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
