[llvm] [Inline][Cloning] Defer constant propagation after phi-nodes resolution (PR #87963)

[Antonio Frighetto via llvm-commits]

================
@@ -823,52 +804,46 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
     }
   }
 
-  // Make a second pass over the PHINodes now that all of them have been
-  // remapped into the new function, simplifying the PHINode and performing any
-  // recursive simplifications exposed. This will transparently update the
-  // WeakTrackingVH in the VMap. Notably, we rely on that so that if we coalesce
-  // two PHINodes, the iteration over the old PHIs remains valid, and the
-  // mapping will just map us to the new node (which may not even be a PHI
-  // node).
-  const DataLayout &DL = NewFunc->getParent()->getDataLayout();
-  SmallSetVector<const Value *, 8> Worklist;
-  for (unsigned Idx = 0, Size = PHIToResolve.size(); Idx != Size; ++Idx)
-    if (isa<PHINode>(VMap[PHIToResolve[Idx]]))
-      Worklist.insert(PHIToResolve[Idx]);
-
-  // Note that we must test the size on each iteration, the worklist can grow.
-  for (unsigned Idx = 0; Idx != Worklist.size(); ++Idx) {
-    const Value *OrigV = Worklist[Idx];
-    auto *I = dyn_cast_or_null<Instruction>(VMap.lookup(OrigV));
-    if (!I)
-      continue;
-
-    // Skip over non-intrinsic callsites, we don't want to remove any nodes from
-    // the CGSCC.
-    CallBase *CB = dyn_cast<CallBase>(I);
-    if (CB && CB->getCalledFunction() &&
-        !CB->getCalledFunction()->isIntrinsic())
-      continue;
-
-    // See if this instruction simplifies.
-    Value *SimpleV = simplifyInstruction(I, DL);
-    if (!SimpleV)
-      continue;
-
-    // Stash away all the uses of the old instruction so we can check them for
-    // recursive simplifications after a RAUW. This is cheaper than checking all
-    // uses of To on the recursive step in most cases.
-    for (const User *U : OrigV->users())
-      Worklist.insert(cast<Instruction>(U));
-
-    // Replace the instruction with its simplified value.
-    I->replaceAllUsesWith(SimpleV);
+  auto GetOriginalValueInVMap = [&](const auto &I) -> Value * {
+    for (const auto &[OrigI, NewI] : VMap) {
+      if (NewI == I)
+        return const_cast<Value *>(OrigI);
+    }
+    return nullptr;
+  };
----------------
antoniofrighetto wrote:

Had the same concern, though in practice I would expect it not to happen that often as most of the times the instruction would be trivially dead? Alternatively, we could iterate over all the instructions of the old function, looking up `VMap` each time to get the new instruction and see if that can be simplified? Not sure if it’d be worth it though. Compile time does not look excessively bad: http://llvm-compile-time-tracker.com/compare.php?from=d34a2c2adb2a4f1dc262c5756d3725caa4ea2571&to=79475c2bf9493e51dcdf46e49b8bd15da326e912&stat=instructions:u. Also, IIRC RPOT was outperforming classic iteration in a few settings (`stage2-O3`).

https://github.com/llvm/llvm-project/pull/87963