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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Having (mostly!) understood what this is doing, the algorithm looks like it's doing the right thing.

High-level ideas would be:

- there are three different representations/sets of APIs exposed: intervals, ordering, and compare. It seems like we only have immediate plans to use compare, and maybe ordering in the future. Maybe we could hide the stuff we're not using, if it's necessary to test it then a narrower interface for testing might be enough? Like `string printIntervalGraph(CFG, int iters)` and then all of Interval could be hidden.
- the data structures for the interval graph are mechanically complicated (trees with differently templated node types etc), and dealing with this incidental complexity and the algorithm complexity is a lot. It may be possible to use simpler data structures, as we don't make use of everything in these ones.



================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:38
+// interval.
 struct CFGInterval {
+  CFGInterval() = default;
----------------
Summarizing an offline discussion, IIUC...
- we build a tree of intervals that represents the whole sequence of interval graphs
- for the current worklist algorithm, we use only the total order and process the first invalid block, the tree is not needed
- the algorithm/data structure could be significantly simplified (no variants, templates, recursion...) if it didn't build the tree, just the interval graphs iteratively until a single node is reached
- however a different, non-worklist algorithm could use the tree information to determine which blocks to visit next. (Given `a (b c d) e`, after `d` we try only `b` or `e`). This avoids expensive "is invalid" checks
- so the motivation for preserving the full interval tree is future-proofing for using a different algorithm

I think it's worth documenting this motivation, and also considering not paying this complexity cost for a feature we aren't adding yet.


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:56
+
+  template <typename Node> struct NodeData {
+    // The block from which the interval was constructed. It is either the
----------------
xazax.hun wrote:
> 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`.
(Very optional idea, in case you like it)

This data structure is complicated: a tree of Intervals with CFGBlocks at the root, and at every level the Intervals have pred/succ edges that form part of one of the intermediate flow graphs.

It seems nice to get rid of the tree if possible (ultimately we're using these for ordering) and the nested pred/succs - we seem to only need these for the top level interval (and in the end, we have one interval and pred/succs are empty.

What if we made this a flat structure, preserving the bracket structure as some kind of back-edge?

```
1  (  2  (  3  4  )  )  5
becomes
1     2     3  4 !3 !2  5
```

AIUI, the dataflow algorithm can basically treat this as a program, we move left to right and `!X` means "if (X is invalidated) goto X".

This would give us something like:
```
using Entry = PointerIntPair<CFGBlock*, 1>; // either an actual block, or a conditional jump
struct Interval { vector<Entry> Sequence; SmallDenseSet<Interval*> Preds, Succs; }
using IntervalGraph = vector<Interval>;
```

I also suspect that this would let us avoid the core algorithms (buildInterval, addIntervalNode) being templated, simply wrap each CFGBlock in a trivial Interval to start with instead. I guess this is OK performance-wise, but the reason not to do it is ending up with those nodes polluting your final data structure?


Anyway, all this is up to you - for me keeping more data/structure around than we're going to use is confusing, but it does seem closer to the textbook.


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:123
+/// This WTO construction is described in Section 4.2 of [Bourdoncle1993].
+std::optional<WeakTopologicalOrdering> getIntervalWTO(const CFG &Cfg);
+
----------------
Naming is confusing here: `getWTO` is the internal function whose API involves intervals, and `getIntervalWTO` be the main function whose API doesn't. I think literally swapping them might be clearer!

in general it's hard to tell which parts of this API people are actually meant to use.
Is it reasonable to wrap everything up to `WeakTopologicalOrdering` in `namespace internal` or so? Or are there places this will be used outside tests?


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:126
+/// Formats `WTO` as a human-readable string.
+std::string formatWTO(const WeakTopologicalOrdering &WTO);
+
----------------
consider instead exposing `printWTO` as `operator<<`.

Then you get formatWTO for free as `llvm::to_string()`, WeakTopologicalOrdering prints reasonably with gtest, you can log it directly, etc.


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:30
+  std::vector<const Node *> Nodes;
+  // llvm::SmallDenseSet<const Node *> Nodes;
+  llvm::SmallDenseSet<const Node *> Successors;
----------------
remove commented out code


================
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();
+}
----------------
xazax.hun wrote:
> We have some convenience wrappers in LLVM: https://llvm.org/doxygen/namespacellvm.html#a086e9fbdb06276db7753101a08a63adf
here specifically `llvm::is_contained(Interval, N)` can be used directly, this function is not needed


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:55
+  for (const Node *S : Header->succs())
     if (S != nullptr)
+      if (auto SID = getID(S); !Partitioned.test(SID)) {
----------------
I'm confused, here and below why are successors allowed to be nullptr?


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:80
     // is included as well.
     bool AllInInterval = true;
+    for (const Node *P : B->preds())
----------------
consider
```
bool AllInInterval = llvm::all_of(B->preds(), [&](const Node *P) {
  return inInterval(P, Interval.Nodes);
});
```
(usually i prefer the plain loop, but tracking booleans is annoying)


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:83
+      if (!inInterval(P, Interval.Nodes)) {
         MaybeSuccessors.push_back(B);
         AllInInterval = false;
----------------
you always add to MaybeSuccessors this exactly once when AllInInterval ends up false, but that's not totally obvious.

Consider moving this below the loop, to the `else` in `if (AllInInterval)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058