[PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064
Artyom.Skrobov at arm.com
Tue Aug 12 01:22:24 PDT 2014
>>> It is true that this inverts the order, but I don't think this
>>> definition of po_iterator is used anywhere except the constructor
>>> for PostOrderInverseCFGView.
>> You're right, it's not; but equally the definition of
>> PostOrderCFGView::po_iterator isn't used anywhere except the
>> constructor for PostOrderCFGView: it's declared privately,
>> and it's not used by any PostOrderCFGView consumers.
> Ok, that makes sense now, but it makes me feel the APIs themselves
> are a bit misleading/confusing. (see below)
Would it help make things a bit clearer if we move this private typedef out
of PostOrderCFGView declaration, into PostOrderCFGView constructor body?
(And the same for the new PostOrderInverseCFGView)
>>> There is no virtual definition of the 'dequeue' method, so
>>> 'dequeue' (which is declared in DataflowWorklistBase) uses
>>> the po_iterator defined in DataflowWorklistBase, which is
>>> the wrong definition for reverse analysis.
>> No, dequeue() uses PostOrderCFGView::iterator, which
>> iterates over the Blocks vector. So, the only factor
>> affecting the order of iteration is how the Blocks vector
>> is initialized in the PostOrderInverseCFGView constructor.
> I see it now. From an API perspective it is very confusing,
> because the name "PostOrderCFGView" implies the iteration
> always is in that order.
Well, that's true: PostOrderCFGView always does it in the "normal" order,
and PostOrderInverseCFGView always does it in the inverse order.
> Also the comment is very misleading if we are doing a reverse
> // Next dequeue from the initial reverse post order. This is the
> // theoretical ideal in the presence of no back edges.
> Technically we'd be doing the opposite thing if this was a
> reverse analysis. The comment if anything is what hung me
> up on thinking it was doing the wrong thing.
The terminology here is a bit muddy, but I took it that "reverse" means
"back to forward", in the sense of using Blocks.rbegin() for the iteration
(which still applies to DataflowWorklistInverse); while "inverse" means
"bottom to top", in the sense of po_iterator visiting predecessors instead
of successors for each block. Assuming this terminology, DataflowWorklist
uses "reverse normal order", and DataflowWorklistInverse uses "reverse
inverse order"; but the comment in question remains valid in both cases.
(I guess I should add the above paragraph into a comment in
More information about the cfe-commits