On Wednesday, March 30, 2016 1:24:09 PM PDT Francisco Jerez wrote: > Kenneth Graunke <[email protected]> writes: > > > There may not be a previous block. In this case, there's no real work > > to do, so just continue on to the next one. > > > > Signed-off-by: Kenneth Graunke <[email protected]> > > --- > > src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp b/src/ mesa/drivers/dri/i965/brw_dead_control_flow.cpp > > index 2c1abaf..116a6c7 100644 > > --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp > > +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp > > @@ -42,6 +42,10 @@ dead_control_flow_eliminate(backend_shader *s) > > > > foreach_block_safe (block, s->cfg) { > > bblock_t *prev_block = block->prev(); > > + > > + if (prev_block->link.is_head_sentinel()) > > + continue; > > + > Heh, the fact this code declares a "prev_block" pointer of type bblock_t > and then asks the object whether it actually is a bblock_t really makes > me itch -- If it's not a bblock_t you're likely relying on UB (At least > the strict aliasing rule seems to be violated).
Yeah, I don't like it either.
I also wrote a follow-on patch that makes block->prev(), block->next(),
check for head/tail sentinels and return a NULL bblock_t pointer. That
way you never return a pointer to the wrong type of memory. This would
become:
if (!prev_block)
continue;
I feel like that's the least surprising API. What do you think?
> I sent a patch to address the same issue a couple of weeks ago which
> doesn't have this problem. It's still pending review:
>
> https://patchwork.freedesktop.org/patch/77199/
I thought about doing that too, but I wasn't convinced at a glance that
NULL'ing the pointers would work. We dereference prev_inst below
without checking if it's NULL. I think the inst->opcode checks
guarantee it's safe. But I thought that simply skipping over the
"no previous block" case with a continue was clearer.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
