Re: [PATCH 1/5] do not corrupt ptrlist while killing unreachable BBs

[Christopher Li <sparse@chrisli.org>]

On Sun, Jul 9, 2017 at 3:26 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>> diff --git a/flow.c b/flow.c
>> index c7161d4..5d2f15a 100644
>> --- a/flow.c
>> +++ b/flow.c
>> @@ -841,7 +841,7 @@ void kill_unreachable_bbs(struct entrypoint *ep)
>>   } END_FOR_EACH_PTR(bb);
>>   PACK_PTR_LIST(&ep->bbs);
>>
>> - repeat_phase &= ~REPEAT_CFG_CLEANUP;
>> + repeat_phase |=  REPEAT_CSE ;
>
> It would be good to add a comment for why the '|= REPEAT_CSE' is needed here.
OK. I will update the patch. Basically removing unreachable dead code will
impact the us


> And not removing the REPEAT_CFG_CLEANUP is an error IMO.
> At the end of the function the CFG *is* clean. If you don't
> clear it here, then what is its meaning?
It is an error as concept or it is an error the program will do wrong
things?

I want to turn REPEAT_CFG_CLEANUP usage pattern similar
to REPEAT_SYMBOL_CLEANUP. Take the a look at

if (repeat_phase & REPEAT_SYMBOL_CLEANUP)
simplify_memops(ep);

The simplify_memops does not clear the REPEAT_SYMBOL_CLEANUP
either. It was clear at the very beginning of the cse repeat loop:

repeat:
repeat_phase = 0;

I just want to be consistent with other usage of the similar flags.

It is also a reflection of my review feedback when you submit the
patch back then. One of the comment I give was that I want to
avoid calling "kill unreachable bb" multiple time within one "ep->bbs"
pass. You said there is no simple way to do it. This patch is actually
what I want back then.

I think when you introduce REPEAT_CFG_CLEANUP you have
different usage in mind. I found the flag clean at beginning of the
CSE loop is conceptually clean as well, it is simpler. The flag
is just a request, it can set more than one times. The action is
perform only once at one CSE pass.

Chris