[PATCH] Inverse post-order traversal for LiveVariables analysis, to recover the performance after r214064

[Artyom Skrobov]

>>>> 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)
>
> It's actually a lot clearer to me now.  But there may be some
> benefits in making this implementation detail truly private,
> especially if it is used in only one place.  That would remove
> some stuff from the header file, cleaning up the interface.

Good, I'll take this implementation detail out of the header file.


>>> 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.
>
> The standard terminology is "forward" and "backwards" analysis.
>  "Normal" implies a bias, when there really isn't one.
>
> The names are also not quite right.  A "forward" analysis uses
> a "reverse postorder", while a "backwards" analysis uses a
> "postorder" traversal.  Thus the names really should be:
>
>  PostOrderCFGView (for backwards analysis)
>  ReversePostOrderCFGView (for forwards analysis)

Yes, I see.


> But I'm wondering about the refactoring here in your patch:
>
> [...]
>
> We're getting specialized behavior via subclassing, but I'm
> wondering if we can make this simpler.  What if we just have
> a "DataflowWorklist" class with one constructor that takes
> an enum indicating a forward or backward analysis.  Based on
> the enum we construct a PostOrderCFGView or a
> ReverseOrderCFGView.  Both of those could subclass some common
> class, say DataflowCFGView, that provides the iterator.
> There is thus no need to duplicate DataflowWorklist.

This is possible, but what do we gain?
How is constructing DataflowWorklist(*cfg, AC, DataflowWorklist::backward)
simpler than constructing DataflowBackwardWorklist(*cfg, AC)?
If I understand you correctly, the syntax for construction is the only
difference between the two approaches.


> Honestly, we may not need two classes for the CFG view either,
> especially if all we are doing is putting the order in the
> Blocks vector.  That seems like we just need a "CFGView" class
> that initializes the Blocks vector based on the order
> specified when the CFGView object is constructed.

PostOrderCFGView is implemented as a ManagedAnalysis, so it has to have a
parameterless static method create(), to be run lazily and cached.
I may be missing something, but I see no way to specify the iteration order
when constructing CFGView, to create two independent instances of it, and to
cache them with respect to their iteration order -- unless the two are
implemented as separate classes.