[Mesa-dev] [PATCH] nir: fix block iterator to not forget fxn->end_block

[Rob Clark]

On Sun, May 8, 2016 at 3:10 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> Before you push this, of like to register my official skepticism as to
> whether or not this is the right fix. Given that the end block is special,
> anyone who depends on iterating over it should know that and handle it
> specially anyway.  I'm pretty sure that block numbering is the only thing
> that actually uses it and fixing that is a 1-line change.

which is why I mentioned "I don't *think* there are any more serious
consequences of not iterating the end_block, but it's probably also a
good idea to not subtly change behaviour compared to the old
fxn-callback based iterator." in the commit msg..

I could go either way on this, given that special casing the block
index #ing is trivial.  And I don't *think* it should matter for
anything else.. but it is changing the behaviour of the iterators.

BR,
-R

> Not a NAK just a thought.
>
> On May 5, 2016 11:41 AM, "Rob Clark" <robdclark at gmail.com> wrote:
>>
>> From: Rob Clark <robclark at freedesktop.org>
>>
>> With the switch to new block iterator macro, we silently stopped
>> iterating over the end-block.  Which caused nir_index_blocks() to not
>> index the end-block.  Resulting in funny looking nir_print's like:
>>
>> impl main {
>>         block block_0:
>>         /* preds: */
>>         intrinsic copy_var () (gl_FragColor at out-temp, gl_Color) ()
>>         intrinsic copy_var () (gl_FragColor, gl_FragColor at out-temp) ()
>>         /* succs: block_0 */
>>         block block_0:
>> }
>>
>> I don't *think* there are any more serious consequences of not iterating
>> the end_block, but it's probably also a good idea to not subtly change
>> behavior compared to the old fxn-callback based iterator.
>>
>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>> ---
>>  src/compiler/nir/nir.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
>> index 867a43c..1f51b31 100644
>> --- a/src/compiler/nir/nir.c
>> +++ b/src/compiler/nir/nir.c
>> @@ -1512,12 +1512,18 @@ nir_block_cf_tree_next(nir_block *block)
>>        return NULL;
>>     }
>>
>> -   nir_cf_node *cf_next = nir_cf_node_next(&block->cf_node);
>> +   nir_cf_node *parent = block->cf_node.parent;
>> +   nir_cf_node *cf_next;
>> +
>> +   if ((parent->type == nir_cf_node_function) &&
>> +       (block == nir_cf_node_as_function(parent)->end_block))
>> +      cf_next = NULL;
>> +   else
>> +      cf_next = nir_cf_node_next(&block->cf_node);
>> +
>>     if (cf_next)
>>        return nir_cf_node_cf_tree_first(cf_next);
>>
>> -   nir_cf_node *parent = block->cf_node.parent;
>> -
>>     switch (parent->type) {
>>     case nir_cf_node_if: {
>>        /* Are we at the end of the if? Go to the beginning of the else */
>> @@ -1532,9 +1538,12 @@ nir_block_cf_tree_next(nir_block *block)
>>     case nir_cf_node_loop:
>>        return nir_cf_node_as_block(nir_cf_node_next(parent));
>>
>> -   case nir_cf_node_function:
>> +   case nir_cf_node_function: {
>> +      nir_function_impl *func = nir_cf_node_as_function(parent);
>> +      if (func->end_block != block)
>> +         return func->end_block;
>>        return NULL;
>> -
>> +   }
>>     default:
>>        unreachable("unknown cf node type");
>>     }
>> --
>> 2.5.5
>>
>