[PATCH] Factor DataflowWorklist out of LiveVariables and UninitializedValues analyses

[Artyom Skrobov]

Hello Jordan and Ted,

 

Is it the right time now to look again at unifying the two
DataflowWorklists?

 

Last month, we've got as far as establishing that the DataflowWorklist in
UninitializedValues may not have been intended to start with all blocks
implicitly enqueued, since it gets some blocks enqueued onto it explicitly
right after the creation; but we couldn't decide without Ted whether the
difference in implementation of the two DataflowWorklists was intentional or
not.

 

 

From: Jordan Rose [mailto:jordan_rose at apple.com] 
Sent: 28 April 2014 17:30
To: Artyom Skrobov
Cc: Ted Kremenek; cfe-commits at cs.uiuc.edu
Subject: Re: [PATCH] Factor DataflowWorklist out of LiveVariables and
UninitializedValues analyses

 

 

On Apr 25, 2014, at 10:43 , Artyom Skrobov <Artyom.Skrobov at arm.com> wrote:

 

> Are you sure this is correct? One worklist treats the entry block as
already analyzed, the other doesn't. One starts with no blocks enqueued, the
other effectively has all blocks enqueued because of the iterators.

 

Well, the patch keeps all tests passing.

How can we trigger the supposed difference in their behaviour?

 

Anyway, the version that "effectively has all blocks enqueued because of the
iterators" had

  worklist.enqueueSuccessors(&cfg.getEntry());

called immediately upon the creation.

What would be the point of enqueuing blocks onto a list that already has all
blocks enqueued?

 

Well, it could have resulted in traversal in a different order, but it
doesn't. I'm not sure that entry actually does anything, since all blocks
will be enqueued at the time. I wonder if this means we're using a less
efficient traversal order than Ted intended...it wouldn't be the first time
we've updated the same code multiple times and accidentally broken the
algorithm. :-(

 

I guess I'm not comfortable applying this until we know what the intended
traversal is for each analysis. Unfortunately Ted (being a manager) probably
will not have time to look at this until after WWDC, i.e. another month from
now.

 

 

 > Why doesn't enqueueSuccessors use enqueueBlock?

 

Thanks for spotting that! Attaching the updated patch.

 

> Since this is only used by classes in the same component of Clang, it
might make sense to put even the header in the lib/ directory. That way it
doesn't show up when other people build tools on top of Clang. Then again,
it is a generally reusable component.

 

Yes, I think it is as generally reusable as PostOrderCFGView it builds upon
(which is, similarly, only used by classes in the same component).

 

Fair enough.

 

Jordan

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140609/ae810ae3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DataflowWorklist.patch
Type: application/octet-stream
Size: 10702 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140609/ae810ae3/attachment.obj>