Re: [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  wrote:
> On Thu, Nov 26, 2015 at 9:22 AM, Emil Velikov  
> wrote:
>> On 25 November 2015 at 22:48, Matt Turner  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

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

[Matt Turner]

On Thu, Nov 26, 2015 at 9:22 AM, Emil Velikov  wrote:
> On 25 November 2015 at 22:48, Matt Turner  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.
Initializing it to WRITEMASK_XYZW doesn't fix the warning in any case.

> 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.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Matt Turner]

On Wed, Nov 25, 2015 at 2:48 PM, Matt Turner  wrote:
> 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.

The attached file reproduces what I'm claiming is a gcc deficiency.
gcc-4.9.3 and gcc-5.2.0 will warn about data0 being uninitialized when
using -Wall. Clang does not.

I'll file a gcc bug.
#define X (1 << 0)

void bar(unsigned data);

void foo(unsigned writemask, unsigned data) {
	unsigned remaining = X;
	unsigned data0;

	if (writemask & X) data0 = data;

	remaining &= ~writemask;
	if (remaining == 0)
	//if (writemask & X)
		bar(data0);
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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  wrote:
> On 3 November 2015 at 18:20, Matt Turner  wrote:
>> On Tue, Nov 3, 2015 at 8:09 AM, Emil Velikov  
>> wrote:
>>> On 3 November 2015 at 00:29, Matt Turner  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(, 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

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

[Emil Velikov]

On 3 November 2015 at 18:20, Matt Turner  wrote:
> On Tue, Nov 3, 2015 at 8:09 AM, Emil Velikov  wrote:
>> On 3 November 2015 at 00:29, Matt Turner  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(, 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.


  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 :-)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Matt Turner]

On Tue, Nov 3, 2015 at 8:09 AM, Emil Velikov  wrote:
> On 3 November 2015 at 00:29, Matt Turner  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(, 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()?

../../../../../../mesa/src/mesa/drivers/dri/i965/brw_vec4.cpp: In
member function ‘bool brw::vec4_visitor::opt_vector_float()’:
../../../../../../mesa/src/mesa/drivers/dri/i965/brw_vec4.cpp:339:92:
warning: ‘imm[3]’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
  vec4_instruction *mov = MOV(inst->dst, brw_imm_vf4(imm[0],
imm[1], imm[2], imm[3]));

 ^
../../../../../../mesa/src/mesa/drivers/dri/i965/brw_vec4.cpp:339:92:
warning: ‘imm[2]’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
../../../../../../mesa/src/mesa/drivers/dri/i965/brw_vec4.cpp:339:92:
warning: ‘imm[1]’ may be used uninitialized in this function
[-Wmaybe-uninitialized]
../../../../../../mesa/src/mesa/drivers/dri/i965/brw_vec4.cpp:339:92:
warning: ‘imm[0]’ may be used uninitialized in this function
[-Wmaybe-uninitialized]

Changing it to brw_imm_vf(*(unsigned *)) generates the same code
as well, showing that the memcpy isn't really happening. It's just
avoiding breaking strict aliasing rules.

Given the (incorrect) warnings, I'm inclined to keep the code as it is.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Emil Velikov]

On 3 November 2015 at 00:29, Matt Turner  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(, 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],)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev