On Sun, May 8, 2016 at 3:10 PM, Jason Ekstrand <ja...@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" <robdcl...@gmail.com> wrote: >> >> From: Rob Clark <robcl...@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@out-temp, gl_Color) () >> intrinsic copy_var () (gl_FragColor, gl_FragColor@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 <robcl...@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 >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev