[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:122
+
+  using BlockOrderTy = llvm::DenseMap<const CFGBlock *, unsigned>;
+  BlockOrderTy BlockOrder;
----------------
Would it make sense to have a vector instead and use block ids to get the order?


================
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
----------------
ymandel wrote:
> xazax.hun wrote:
> > I wonder if we still want to somehow enforce that within a loop/interval the we visit nodes in the RPO order.
> Starting with RPO should make this algorithm cheaper (and maybe simpler!).  But, RPO construction isn't free. So, I think it is a matter of cost -- compare generating the RPO first and then using that here vs. just using this directly.  I can put a note in the implementation as FIXME. WDYT?
Sounds good to me.


================
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:
> > 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. 
> I ended up changing to a deque after almost making the same mistake again. vector is just not safe when you want to be taking addresses. :) I've dropped the use of IDs and added comments. Let me know if this clear enough.
Looks good, thanks! I agree that the deque solution looks cleaner, probably we should get it first right and we can optimize later if there is a need for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058