[PATCH] Factor DataflowWorklist out of LiveVariables and UninitializedValues analyses

[Artyom Skrobov]

Another ping on this patch dating from back in April..


-----Original Message-----
From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
Sent: 04 July 2014 09:24
To: 'Ted Kremenek'
Cc: 'Jordan Rose'; 'cfe-commits at cs.uiuc.edu'
Subject: RE: [PATCH] Factor DataflowWorklist out of LiveVariables and
UninitializedValues analyses

Hello Ted,

Did you, by any chance, have time to look at DataflowWorklist
implementations again?


-----Original Message-----
From: Artyom Skrobov [mailto:Artyom.Skrobov at arm.com] 
Sent: 23 June 2014 20:27
To: 'Ted Kremenek'
Cc: Jordan Rose; cfe-commits at cs.uiuc.edu
Subject: RE: [PATCH] Factor DataflowWorklist out of LiveVariables and
UninitializedValues analyses

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?