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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:56-64
+  template <typename Node> struct NodeData {
+    // The block from which the interval was constructed. It is either the
+    // graph's entry block or has at least one predecessor outside the interval.
+    const Node *Header;
+    std::vector<const Node *> Nodes;
+  };
+  using IntervalData = std::variant<NodeData<CFGBlock>, NodeData<CFGInterval>>;
----------------
Correct me if I'm wrong but I have the impression that `CFGInterval` might either contain only `CfgBlock`s or other `CfgInterval`s, but these might never be mixed. The current representation does allow such mixing. In case do not need that, I think it might be cleaner to make `CfgInterval` itself templated and remove the `std::variant`.


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:101-103
+/// groups topologically. As a result, the blocks in a series of loops are
+/// ordered such that all nodes in loop `i` are earlier in the order than nodes
+/// in loop `j`. This ordering, when combined with widening, bounds the number
----------------
I wonder if we still want to somehow enforce that within a loop/interval the we visit nodes in the RPO order.


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:36
+bool inInterval(const Node *N, std::vector<const Node *> &Interval) {
+  return std::find(Interval.begin(), Interval.end(), N) != Interval.end();
+}
----------------
We have some convenience wrappers in LLVM: https://llvm.org/doxygen/namespacellvm.html#a086e9fbdb06276db7753101a08a63adf


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:121
+    Index.emplace(N, ID);
+  Graph.Intervals.emplace_back(ID, Header, std::move(Data.Nodes));
 }
----------------
ymandel wrote:
> xazax.hun wrote:
> > It would probably take for me some time to understand what is going on here, but I think this part might worth a comment. In particular, I have the impression that `Graph.Intervals` is the owner of all the intervals. But we use pointers to refer to these intervals. What would guarantee that those pointers are not being invalidated by this `emplace_back` operations?
> Excellent question, not least because I *wasn't* careful the first around and indeed ran up against this as a memory bug. :) That's the reason I added IDs, so that I could _avoid_ taking the addresses of elements until after we finish growing the vector. See lines 148-165: after we finish building the intervals, we go through and take addresses.
> 
> I can add comments to this effect, but perhaps the safe option is to use a std::dequeue. What do you think?
Oh, thanks! I don't have a strong preference between a comment or a deque. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058