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: > > 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. I'm not sure we allocate enough of these in the course of a compilation for the one extra word in a `std::vector` to matter, but I won't press the point. ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:17 +/// implemented as a bit vector, wherein each bit corresponds to one edge in +/// the edge array. This implies a lower bound of 64x spacial improvement +/// over, e.g., an llvm::DenseSet or llvm::SmallSet. It also means that ---------------- "spatial" ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:41 +class ImmutableGraph { + using Traits = GraphTraits<ImmutableGraph<NodeValueT, EdgeValueT> *>; + template <typename> friend class ImmutableGraphBuilder; ---------------- I think this self-reference to `ImmutableGraph` dropped the `SizeT` parameter. ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:73 + // The end of this Node's edges is the beginning of the next node's edges. + const Edge *edges_end() const { return (this + 1)->Edges; } + ArrayRef<Edge> edges() const { ---------------- Seems like you also want to add a comment here that we know we will never be asked for `edges_end` for the last stored node -- that is, we know that `this + 1` always refers to a valid Node (which is presumably a dummy/sentinel) ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:79 + +protected: + ImmutableGraph(std::unique_ptr<Node[]> Nodes, std::unique_ptr<Edge[]> Edges, ---------------- Why "protected" rather than "private"? Usually seeing "protected" makes me think subclassing is expected, but that doesn't seem to be the case here. ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:117 + NodeSet(const ImmutableGraph &G, bool ContainsAll = false) + : G{G}, V{static_cast<unsigned>(G.nodes_size()), ContainsAll} {} + bool insert(const Node &N) { ---------------- How do we know that a value of `size_type` (aka `SizeT`) can be cast to `unsigned` without truncation? ================ Comment at: llvm/lib/Target/X86/ImmutableGraph.h:298 + static_assert( + std::is_base_of<ImmutableGraph<node_value_type, edge_value_type>, + GraphT>::value, ---------------- this will also break if a non-default `SizeT` is provided. Maybe a good argument to just leave out `SizeT` for now, and it can be added in the future as needed? ================ Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:113 + NumFences(NumFences), NumGadgets(NumGadgets) {} + MachineFunction &getMF() { // FIXME: This function should be cleaner + for (const Node &N : nodes()) ---------------- Cleaner how? ================ Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:233 + if (!STI->useLVILoadHardening() || !STI->is64Bit()) + return false; // FIXME: support 32-bit + ---------------- If the user requests hardening and we can't do it, it seems better to fail loudly so they don't accidentally deploy an unmitigated binary. 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