> On Aug 12, 2014, at 1:22 AM, Artyom Skrobov <[email protected]> wrote: > > >>>> 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)
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. > > >>>> 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. 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) see: http://en.wikipedia.org/wiki/Data-flow_analysis <http://en.wikipedia.org/wiki/Data-flow_analysis> ("the order matters") > > >> Also the comment is very misleading if we are doing a reverse >> analysis: >> >> // 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 > DataflowWorklist.h?) Per my comment above, let's keep with the standard terminology: "forward" and "backwards". But I'm wondering about the refactoring here in your patch: > - void sortWorklist(); > +class DataflowWorklist : public DataflowWorklistBase { > +public: > + DataflowWorklist(const CFG &cfg, AnalysisDeclContext &Ctx) > + : DataflowWorklistBase(cfg, *Ctx.getAnalysis<PostOrderCFGView>()) {} > }; > > +class DataflowWorklistInverse : public DataflowWorklistBase { > +public: > + DataflowWorklistInverse(const CFG &cfg, AnalysisDeclContext &Ctx) > + : DataflowWorklistBase(cfg, *Ctx.getAnalysis<PostOrderInverseCFGView>()) > {} > +}; > + > } // end clang namespace > 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. 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. The nice thing about this is that we don't get an expansion of classes and concepts. All we have is a worklist that consults a CFG ordering.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
