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

[Connor Abbott]

On Sun, May 8, 2016 at 4:09 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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.

While it is changing the behavior, and I should've caught this while
writing the series (sorry!), I also have a mild preference for the new
behavior for the same reasons that Jason gave, and it would seem silly
to apply this patch and then immediately revert it. I think the best
thing to do would be to fixup the one special case and then document
the new behavior.

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