sconstab 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]; ---------------- mattdr wrote: > 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? A statically sized array (e.g., std::array) is insufficient because the size in this case is not compiler determinable; a dynamically sized and dynamically resizable array (e.g., std::vector) is sufficient but overly costly; a dynamically sized and dynamically //unresizable// array is sufficient and has minimal cost. ================ 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. ---------------- 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. ================ 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 ---------------- 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. ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:40 + +template <typename NodeValueT, typename EdgeValueT, typename SizeT = int> +class ImmutableGraph { ---------------- mattdr wrote: > 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? I suspect that there may be more uses for this data structure and that eventually it may migrate to ADT. I have SizeT as a template argument because I found it plenty sufficient to have `int` as the size parameter for the array bounds, but I suspect other uses may require `size_t`. 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