[PATCH] Factor DataflowWorklist out of LiveVariables and UninitializedValues analyses
Artyom Skrobov
Artyom.Skrobov at arm.com
Mon Jun 23 12:26:37 PDT 2014
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?
More information about the cfe-commits
mailing list