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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
