Kenneth Graunke <[email protected]> writes: > 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
That would definitely be a good thing to change regardless of whether we go with one patch or the other. bblock_t::prev/next return a bblock_t pointer so the return value should better point to an object of type bblock_t if non-null -- Otherwise you're encouraging people to make this sort of mistakes. > way you never return a pointer to the wrong type of memory. This would > become: > > if (!prev_block) > continue; > Yeah, this hunk from my patch would also become unnecessary in that case: | - bblock_t *prev_block = block->prev(); | + bblock_t *const prev_block = block->num ? block->prev() : NULL; > 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. The reason the null checks are unnecessary is that the instruction being an ENDIF or ELSE guarantees that there is a preceding instruction, so it seems redundant to verify that prev_inst is non-NULL in addition.
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
