[PATCH] D101494: [SimplifyCFG] Ignore ephemeral values when counting insts for threading

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.


================
Comment at: llvm/lib/Analysis/CodeMetrics.cpp:131
+
+  completeEphemeralValues(Visited, Worklist, EphValues);
+}
----------------
nikic wrote:
> So there's two potential compile-time problems here: The first is that while this starts off with assume inside the block, it may scan uses outside the block as well. Additionally it scans over all assumes in order to find those in the block.
> 
> I'm not sure whether this is really important in practice, but having seen pathological ephemeral value collection during inlining, I'm being a bit cautious here.
> 
> I think for the particular case it is used for here, it might make the most sense to not collect ephemeral values upfront, instead compute them on the fly. We can do this by changing the direction of the instruction walk from end to start, and then doing something like:
> 
> ```
> SmallPtrSet<const Value *, 32> EphValues;
> auto IsEphemeral = [&](const Value *V) {
>   if (isa<AssumeInst>(V))
>     return true;
>   return isSafeToSpeculativelyExecute(V) &&
>          all_of(V->users(), [&](const User *U) { return EphValues.count(U); });
> };
> for (Instruction &I : reverse()) {
>   if (IsEphemeral(&I))
>     EphValues.insert(&I);
> 
>   // Otherwise normal code.
> }
> ```
> 
> This also has the advantage that we don't need to compute any ephemeral values past the ten or so instructions we look at.
I was concerned about this too at first. I collected some stats for a large application build and found that on average there were very few assumptions being checked. That being said, pathological cases could occur, and I agree that it is straightforward to reverse the loop and collect on demand, so I changed it to do that.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2440
   for (Instruction &I : BB->instructionsWithoutDebug()) {
     if (Size > MaxSmallBlockSize)
       return false; // Don't clone large BB's.
----------------
One issue with the reversed loop is that the Size is checked against the limit the iteration after it is incremented. When iterating in forward order, this means that the branch is not counted against the limit, since the loop exits before the subsequent check. But with the reversed order it gets counted and the test started failing since we no longer did the threading. I decided to consolidate the Size increment and check to make it more consistent, and simply bumped up the default limit and the one used in the test so that there is no change to the status quo in terms of non-ephemeral values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101494