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

[Craig Topper via Phabricator via llvm-commits]

craig.topper marked 2 inline comments as done.
craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:285
+  std::unique_ptr<Edge[]> Edges;
+  size_type EdgesSize;
+};
----------------
sconstab wrote:
> @craig.topper It now occurs to me that these fields should probably be reordered to:
> ```
>   std::unique_ptr<Node[]> Nodes;
>   std::unique_ptr<Edge[]> Edges;
>   size_type NodesSize;
>   size_type EdgesSize;
> ```
> The current ordering will cause internal fragmentation.
> 
> Old ordering:
> ```
> static_assert(sizeof(ImmutableGraph<T, V>) == 32);
> ```
> New ordering:
> ```
> static_assert(sizeof(ImmutableGraph<T, V>) == 24);
> ```
> With vectors instead of arrays:
> ```
> static_assert(sizeof(ImmutableGraph<T, V>) == 48);
> ```
I noticed that too. I just didn't focus on it since we only ever one in memory at a time. I'll change in my next update.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:307
+public:
+  using NodeRef = size_type;
+
----------------
sconstab wrote:
> Just noticed that `ImmutableGraphBuilder` and `ImmutableGraph` have non-identical types called `NodeRef`. Suggest renaming this one to `BuilderNodeRef`.
NodeRef is in the Traits class not the ImmutableGraph, but I will rename the builder one.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75936/new/

https://reviews.llvm.org/D75936