[PATCH] D152263: [clang][CFG] Add support for partitioning CFG into intervals.

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

xazax.hun added inline comments.


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:35
+
+  std::set<const CFGBlock *> Blocks;
+
----------------
Nit: I wonder if we want something like `llvm::DenseSet` when we use smaller types like pointers. Same for `Successors`.


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:26
+
+  std::queue<const CFGBlock *> Worklist;
+  for (const CFGBlock *S : Header.succs())
----------------
Is it possible we end up adding the same node to the queue multiple times? Is that desirable or do we want to make sure we only have each node at most once?


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:37
+  // successors.
+  std::vector<const CFGBlock *> MaybeSuccessors;
+
----------------
Same question here, is it possible we might end up adding the same nodes multiple times? 


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:46-47
+
+    // Check whether all predecessors are in the interval, in which case `B`
+    // is included as well.
+    bool AllInInterval = true;
----------------
I wonder if this approach is correct. Consider the following scenario:

```
   A
  / \
 B   C
 |   |
 |   D
  \ /
   E
```

In the BFS, we might visit: ABCED. Since we visit `E` before `D`, we might not recognize that `E` is part of the interval. Do I miss something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152263