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

[Kenneth Graunke]

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.

Patches 1-8 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
(and you can ignore my comment on patch 4 - I'm happy to see that done in 
later patches - no need to block this any further on trivialities)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140531/5f68791c/attachment.sig>