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

[Scott Constable via Phabricator via llvm-commits]

sconstab added inline comments.


================
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());
----------------
mattdr wrote:
> 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.
Thanks for pointing this out. I don't know why my intuition led me to use `std::max`. The minimum multi-cut algorithm would, in essence, also be adding the weights. And I think you're also correct that edges that have already been cut can be treated as having cost `0`.

I implemented these changes, and measured (1) a small decrease in the number of `LFENCE`s inserted, (2) no significant performance difference, and (3) no exposed LVI vulnerabilities.


================
Comment at: llvm/lib/Target/X86/X86LoadValueInjectionLoadHardening.cpp:706
+          IngressCutCost < EgressCutCost ? IngressEdges : EgressEdges;
+      llvm::for_each(EdgesToCut, [&](EdgeRef E) { CutEdges.insert(*E); });
     }
----------------
mattdr wrote:
> EdgeSet has `|=` for union, maybe that would be better here
`|=` is not overloaded for an array-like container on the RHS. Unless you think that `EdgesToCut` should also be a BitVector?


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

https://reviews.llvm.org/D84471