xazax.hun added inline comments.
================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:56-64
+ template <typename Node> struct NodeData {
+ // The block from which the interval was constructed. It is either the
+ // graph's entry block or has at least one predecessor outside the
interval.
+ const Node *Header;
+ std::vector<const Node *> Nodes;
+ };
+ using IntervalData = std::variant<NodeData<CFGBlock>, NodeData<CFGInterval>>;
----------------
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`.
================
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
----------------
I wonder if we still want to somehow enforce that within a loop/interval the we
visit nodes in the RPO order.
================
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();
+}
----------------
We have some convenience wrappers in LLVM:
https://llvm.org/doxygen/namespacellvm.html#a086e9fbdb06276db7753101a08a63adf
================
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:
> > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153058/new/
https://reviews.llvm.org/D153058
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
