[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 5 inline comments as done.
ymandel added a comment.

Thanks for the helpful suggestions!



================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:104
+/// intervals) if and only if it is reducible (its limit flow graph has one
+/// node). Returns `nullop` when `Cfg` is not reducible.
+///
----------------
sammccall wrote:
> sammccall wrote:
> > nit: nullopt
> what do we expect to do in practice when the CFG is not reducible? or do we expect that to never happen?
> 
> (mostly wondering if we need a fallback)
good question -- I'm not really sure what to expect. In practice, any structured function will yield a reducible graph. But, creative use of goto's can undermine that. So, my guess is that we will not encounter these in practice, but I don't know for sure.

As for fallback, RPO doesn't rely on reducibility. So, clients are free to use that instead when this fails.


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:105
+void fillIntervalNode(CFGIntervalGraph &Graph,
+                      std::map<const Node *, CFGIntervalNode *> &Index,
+                      std::queue<const Node *> &Successors,
----------------
sammccall wrote:
> std::map of pointers is a bit suspicious, densemap?
I went with a vector mapping (implicitly) from Node IDs, like I did for the WTO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153058/new/

https://reviews.llvm.org/D153058