[PATCH] D75936: Add a Pass to X86 that builds a Condensed CFG for Load Value Injection (LVI) Gadgets [4/6]

[Scott Constable via Phabricator via llvm-commits]

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