mattdr added inline comments.
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:318
+ }
+ auto *VertexArray = new Node[VertexSize + 1 /* terminator node */];
+ auto *EdgeArray = new Edge[EdgeSize];
----------------
sconstab wrote:
> mattdr wrote:
> > As a general rule `new` is a code-smell in modern C++. This should be a
> > `vector`.
> @mattdr I do agree with the general rule. I also think that in this case
> where the structure is immutable, std::vector is wasteful because it needs to
> keep separate values for the current number of elements and the current
> capacity. At local scope within a function the unneeded value would likely be
> optimized away, but then there would be an awkward handoff to transfer the
> data from the vector to the array members.
>
> I would not want to see the array members changed to vectors, unless LLVM
> provides an encapsulated array structure that does not need to grow and
> shrink.
So, first: I'm glad you removed the unnecessary use of `new[]` here and the
corresponding (and error-prone!) use of `delete[]` later. That removes a memory
leak LLVM won't have to debug.
You suggest here that something other than `std::vector` would be more
efficient. If so, would `std::array` suffice? If not, can you explain why
static allocation is impossible but dynamic allocation would be too expensive?
================
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.
----------------
erm, "terrific"? If there's a substantive argument w.r.t. cache locality etc.,
please make it explicit.
================
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
----------------
"extraordinarily" is, again, not a useful engineering categorization. Please
restrict comments to describing quantifiable claims of complexity.
================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:40
+
+template <typename NodeValueT, typename EdgeValueT, typename SizeT = int>
+class ImmutableGraph {
----------------
Every template argument for a class represents combinatorial addition of
complexity for the resulting code. Why do each of these template arguments need
to exist? in particular, why does SizeT need to exist?
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
