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

Reply via email to