[Mesa-dev] [PATCH 05/19] i965/fs: Loop from 0 to inst->sources, not 0 to 3.

[Ian Romanick]

On 06/02/2014 05:49 PM, Kenneth Graunke wrote:
> On Monday, June 02, 2014 03:48:23 PM Ian Romanick wrote:
>> On 05/31/2014 10:11 PM, Kenneth Graunke wrote:
>>> On Tuesday, May 27, 2014 06:47:36 PM Matt Turner wrote:
>>> [snip]
>>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp 
>>> b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>>> index 069b60f..a1aff21 100644
>>>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>>> @@ -353,7 +353,7 @@ fs_visitor::try_constant_propagate(fs_inst *inst, 
>>> acp_entry *entry)
>>>>     if (entry->src.file != IMM)
>>>>        return false;
>>>>  
>>>> -   for (int i = 2; i >= 0; i--) {
>>>> +   for (int i = inst->sources - 1; i >= 0; i--) {
>>>>        if (inst->src[i].file != entry->dst.file ||
>>>>            inst->src[i].reg != entry->dst.reg ||
>>>>            inst->src[i].reg_offset != entry->dst.reg_offset ||
>>>
>>> I am glad to see that you're using a signed integer type here.  For 
>>> BRW_OPCODE_NOP, SHADER_OPCODE_PLACEHOLDER_HALT, and so on, inst->sources 
> could 
>>> conceievably be 0.  (Today, you set it to 3 for some reason, but I could 
> see 
>>> someone coming along and "fixing" that later...)
>>>
>>> If you had used unsigned types, this would translate into i = 4294967296; 
> i >= 
>>> 0; i--, which would take forever.  With signed types, it works, and no one 
>>> should get bitten by that bug.
>>
>> Consistency of types is good.  Some folks build with additional
>> warnings, and the flood of
>>
>> main/bufferobj.c:2855:18: warning: comparison between signed and unsigned 
> integer expressions [-Wsign-compare]
>>
>> can hide legit bugs. :(
> 
> I'm not advocating against leaving compiler warnings.  I'm advocating against 
> naively changing it to unsigned in order to "shut up" those warnings, and 
> actually /introduce/ bugs.

Of course!  Me neither. :)

> Here, the right solution is probably to use a signed counter, and explicitly 
> cast uint -> int to shut up the compiler.

I guess the part I was thinking but didn't say is why not just use
signed in all these places?  I don't care too much either way... I just
want less bugs and less warning noise. :)

> --Ken


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140603/85e4a241/attachment.sig>