[PATCH] Factor DataflowWorklist out of LiveVariables and UninitializedValues analyses

[Ted Kremenek]

Hi Artyom,

You're absolutely right.  I didn't quite notice that enqueuePredecessors and enqueueSuccessors are fundamentally different.  This is functionally equivalent to what we have now.  I think I misread that the same algorithm was used for both forward and reverse analysis, which is not the case.  That's a *very* subtle difference, which was more distinct when the code was in separate places.  By harmonizing the implementations (thank you!) that distinction is a bit loss and not very obvious.

I have two comments:

(1) I think the patch can go in as is, but I'd like two comments, one above enqueueSuccessors and enqueuePredecessors respectively that document these algorithmic differences.  It doesn't need to be an essay, but I worry about these algorithmic differences being lost.

(2) I think the approach we took for forward analysis could be applied to reverse analysis as well, especially now that you've harmonized the implementations.  That can go in a subsequent patch.  If I get a chance, I'll take a stab at doing it.

Thank you so much for reviewing this.  I think this can go in.

Cheer,s
Ted

On Jun 23, 2014, at 12:26 PM, Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:

> Ted, thank you for your clarifications, and for the overview of how this
> code developed historically.
> 
> 
>> Looking at your patch, you've used the data flow analysis in
>> LiveVariables.cpp as your baseline implementation for both of
>> them, or you've merged the two together.  This will work, and
>> produce correct results, but may be dramatically slower for
>> -Wuninitialized.  We don't want to be resorting the entire
>> worklist after enqueueing successors.
> 
> No, this is not the case: the implementation from LiveVariables.cpp had only
> called sortWorklist() from its enqueuePredecessors(), and so does my merged
> implementation. It doesn't re-sort the worklist in enqueueSuccessors().
> 
> runUninitializedVariablesAnalysis() doesn't use enqueuePredecessors(), as it
> hadn't used it before; so this analysis still doesn't involve any sorting.
> 
> 
>> Instead, we should probably use the approach in
>> UninitializedValues.cpp, which uses the real worklist as a
>> prioritization worklist, and then consults the reverse post order
>> second if the worklist is empty.  Your tests are passing because
>> both implementations are correct, but possibly quite a bit slower
>> because it constantly resorts the worklist.
> 
> Indeed, the code for dequeue() in my merged implementation is identical to
> that in UninitializedValues.cpp, and it first tries to dequeue from the
> worklist, then tries the reverse post order.
> 
> 
>>     Switch -Wuninitialized to use a reverse-post order traversal as
>>     an initial baseline for enqueued blocks, but use a simple DFS stack
>>     for propagating changes quickly up back edges.
>> 
>>     This provides a 3.5% reduction in -fsyntax-only time on sqlite3.c.
>> 
>> That should provide a good test case for you to determine if your
>> patch regresses performance.
> 
> I have now tried running "clang -Wuninitialized -fsyntax-only sqlite3.c"
> with and without my patch, and it takes the same time in both cases.
> 
> Would you please take a second look to see if I'm missing any problems with
> my patch?
> 
> 
> 
>