sconstab added a comment.
Overall, the restyling by @craig.topper looks much better than what I had
committed before. I agree that `std::unique_ptr<T *>` is the right "container"
in this circumstance. And the addition of `ArrayRef<>` accessors is also a nice
touch. A few extra inline comments.
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:13
+/// The advantages to this implementation are two-fold:
+/// 1. Iteration and traversal operations should experience terrific caching
+/// performance.
----------------
sconstab wrote:
> mattdr wrote:
> > erm, "terrific"? If there's a substantive argument w.r.t. cache locality
> > etc., please make it explicit.
> This is valid. I will reword.
"Iteration and traversal operations benefit from cache locality."
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:16
+/// 2. Set representations and operations on nodes and edges become
+/// extraordinarily efficient. For instance, a set of edges is implemented
as
+/// a bit vector, wherein each bit corresponds to one edge in the edge
----------------
sconstab wrote:
> mattdr wrote:
> > "extraordinarily" is, again, not a useful engineering categorization.
> > Please restrict comments to describing quantifiable claims of complexity.
> AFAIK there is not a precise engineering term for "tiny O(1)." Nonetheless I
> will reword.
"Operations on sets of nodes/edges are efficient, and representations of those
sets in memory are compact. For instance..."
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:84
+ : Nodes(std::move(Nodes)), NodesSize(NodesSize), Edges(std::move(Edges)),
+ EdgesSize(EdgesSize) {}
+ ImmutableGraph(const ImmutableGraph &) = delete;
----------------
After the members are reordered, this list must also be reordered.
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:103
+
+ class NodeSet {
+ friend class iterator;
----------------
This had not occurred to me until now, but a lot of code is shared between
`NodeSet` and `EdgeSet`. Maybe a template could reduce the redundancy?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75936/new/
https://reviews.llvm.org/D75936
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
