[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 6 inline comments as done.
craig.topper added inline comments.


================
Comment at: llvm/lib/Target/X86/ImmutableGraph.h:366
+    std::vector<size_type> TrimmedNodes(TrimNodes.size());
+    for (size_type I = 0; I < TrimNodes.size(); ++I) {
+      TrimmedNodes[I] = TrimmedNodesSoFar;
----------------
mattdr wrote:
> Two comments here would make the code significantly easier to understand:
> 1. Note that we're using `.size()` here rather than `.count()`, so we're actually iterating over *all* Node indices, not just the ones to be trimmed
> 2. The `TrimmedNodes` vector maps indices in the original NodeSet to the number of `Node`s before that index that have been trimmed by that index, to allow later code to map elements to their new position in a dense array with the trimmed items removed
I've stopped using TrimmedNodes.size() and TrimEdges.size() in favor of the size methods from the graph which should make things more obvious.

I renamed TrimmedNodes to RemappedNodeIndex and stored the new index rather than the adjustment needed. I'm also changed it to walk nodes instead of indices so we don't have to translate to Node to make the contains call.

I also removed the NewNumEdges count_if and the if statement around the edge loop from the loop below. I don't think that provided any value and just complicated the code.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:8626
   if (!Ld || (NumElts - NumUndefElts) <= 1) {
+    dbgs() << "ctopper1\n";
     APInt SplatValue, Undef;
----------------
Oops this snuck in from something else I was experimenting with in my repo earlier. I'll remove.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:506
+    const PseudoSourceValue *PSV = MMO->getPseudoValue();
+    if (PSV && PSV->kind() == K && MMO->isLoad())
+      return true;
----------------
mattdr wrote:
> It seems very weird to make this a template argument rather than just, like, a regular argument.
Agreed. I've remove the template argument and made it a static function instead of a method since it doesn't use anything from the class.


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

https://reviews.llvm.org/D75936