[PATCH] D84471: [X86] Fix for ballooning compile times due to Load Value Injection (LVI) mitigations

[Matthew Riley via Phabricator via llvm-commits]

mattdr added a comment.

Glad that we could make this simpler without making the resulting code much slower!



================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:682-684
+  // mitigated. This implies either:
+  // (1) cutting all egress CFG edges from the gadget source, or
+  // (2) cutting all ingress CFG edges to the gadget sink.
----------------
nit: I spent some time trying to figure out how these are "implied" by the requirement and I don't think they are. Let's update the comment to make it clear we're describing one approach that happens to meet the requirement at a sweet spot of simplicity and speed. 


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:699-702
+      for (const auto *EgressEdge : EgressEdges)
+        EgressCutCost = std::max(EgressCutCost, EgressEdge->getValue());
+      for (const auto *IngressEdge : IngressEdges)
+        IngressCutCost = std::max(IngressCutCost, IngressEdge->getValue());
----------------
Two things:
1. What's the intuition for aggregating with "max" here rather than, say, summing the weights?

2. It seems like we should be taking CutEdges into account somehow here. If we've already decided to cut an expensive edge, the *incremental* cost of cutting it again is zero and we shouldn't be including it here.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:706
+          IngressCutCost < EgressCutCost ? IngressEdges : EgressEdges;
+      llvm::for_each(EdgesToCut, [&](EdgeRef E) { CutEdges.insert(*E); });
     }
----------------
EdgeSet has `|=` for union, maybe that would be better here


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

https://reviews.llvm.org/D84471