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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 2 inline comments as not done.
ymandel added inline comments.


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:26
+
+  std::queue<const CFGBlock *> Worklist;
+  for (const CFGBlock *S : Header.succs())
----------------
xazax.hun wrote:
> 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?
Added an answer in comments, but repeating here in case you want to discuss further:

  // The worklist *may* contain duplicates. We guard against this possibility by
  // checking each popped element for completion (that is, presence in
  // `Partitioned`). We choose this approach over using a set because the queue
  // allows us to flexibly insert and delete elements while iterating through
  // the list. A set would require two separate phases for iterating and
  // mutation.


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:37
+  // successors.
+  std::vector<const CFGBlock *> MaybeSuccessors;
+
----------------
xazax.hun wrote:
> Same question here, is it possible we might end up adding the same nodes multiple times? 
Added an answer in comments, but repeating here in case you want to discuss further:

  // It may contain duplicates -- ultimately, all relevant elements
  // are added to `Interval.Successors`, which is a set.


================
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;
----------------
xazax.hun wrote:
> 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?
> 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?

When we add `D` to the interval, we'll push `E` onto the queue again (lines 58-59). The second time that `E` is considered it will have both successors in the interval and will be added as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152263