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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/CodeMetrics.cpp:131
+
+  completeEphemeralValues(Visited, Worklist, EphValues);
+}
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2449
     // We will delete Phis while threading, so Phis should not be accounted in
-    // block's size
-    if (!isa<PHINode>(I))
+    // block's size. Ditto for ephemeral values which will also be deleted.
+    if (!isa<PHINode>(I) && !EphValues.count(&I))
----------------
This comment is confusing, in that ephemeral values will not be deleted while threading (unlike phis). They will only be deleted during codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101494