Merge pull request #78136 from eeckstein/fix-ssa-updater-2

eeckstein · web-flow · commit 0c860f8abaa7 · 2024-12-12T20:59:02.000+01:00
LoopRotate: remove redundant phis after ssa-update
diff --git a/SwiftCompilerSources/Sources/Optimizer/PassManager/PassRegistration.swift b/SwiftCompilerSources/Sources/Optimizer/PassManager/PassRegistration.swift
@@ -140,5 +140,5 @@ private func registerSwiftAnalyses() {
 
 private func registerUtilities() {
   registerVerifier()
-  registerGuaranteedPhiUpdater()
+  registerPhiUpdater()
 }
diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/CMakeLists.txt b/SwiftCompilerSources/Sources/Optimizer/Utilities/CMakeLists.txt
@@ -9,7 +9,6 @@
 swift_compiler_sources(Optimizer
   AccessUtilsTest.swift
   AddressUtils.swift
-  GuaranteedPhiUpdater.swift
   BorrowUtils.swift
   SpecializationCloner.swift
   DiagnosticEngine.swift
@@ -22,6 +21,7 @@ swift_compiler_sources(Optimizer
   LocalVariableUtils.swift
   OptUtils.swift
   OwnershipLiveness.swift
+  PhiUpdater.swift
   SSAUpdater.swift
   StaticInitCloner.swift
   Test.swift
diff --git a/SwiftCompilerSources/Sources/Optimizer/Utilities/PhiUpdater.swift b/SwiftCompilerSources/Sources/Optimizer/Utilities/PhiUpdater.swift
@@ -1,4 +1,4 @@
-//===--- GuaranteedPhiUpdater.swift ---------------------------------------===//
+//===--- PhiUpdater.swift -------------------------------------------------===//
 //
 // This source file is part of the Swift.org open source project
 //
@@ -152,13 +152,100 @@ private func addEnclosingValues(
   return true
 }
 
-func registerGuaranteedPhiUpdater() {
-  BridgedUtilities.registerGuaranteedPhiUpdater(
+/// Replaces a phi with the unique incoming value if all incoming values are the same:
+/// ```
+///   bb1:
+///     br bb3(%1)
+///   bb2:
+///     br bb3(%1)
+///   bb3(%2 : $T):   // Predecessors: bb1, bb2
+///     use(%2)
+/// ```
+/// ->
+/// ```
+///   bb1:
+///     br bb3
+///   bb2:
+///     br bb3
+///   bb3:
+///     use(%1)
+/// ```
+///
+func replacePhiWithIncomingValue(phi: Phi, _ context: some MutatingContext) -> Bool {
+  if phi.predecessors.isEmpty {
+    return false
+  }
+  let uniqueIncomingValue = phi.incomingValues.first!
+  if !uniqueIncomingValue.parentFunction.hasOwnership {
+    // For the SSAUpdater it's only required to simplify phis in OSSA.
+    // This avoids that we need to handle cond_br instructions below.
+    return false
+  }
+  if phi.incomingValues.contains(where: { $0 != uniqueIncomingValue }) {
+    return false
+  }
+  if let borrowedFrom = phi.borrowedFrom {
+    borrowedFrom.uses.replaceAll(with: uniqueIncomingValue, context)
+    context.erase(instruction: borrowedFrom)
+  } else {
+    phi.value.uses.replaceAll(with: uniqueIncomingValue, context)
+  }
+
+  let block = phi.value.parentBlock
+  for incomingOp in phi.incomingOperands {
+    let existingBranch = incomingOp.instruction as! BranchInst
+    let argsWithRemovedPhiOp = existingBranch.operands.filter{ $0 != incomingOp }.map{ $0.value }
+    Builder(before: existingBranch, context).createBranch(to: block, arguments: argsWithRemovedPhiOp)
+    context.erase(instruction: existingBranch)
+  }
+  block.eraseArgument(at: phi.value.index, context)
+  return true
+}
+
+/// Replaces phis with the unique incoming values if all incoming values are the same.
+/// This is needed after running the SSAUpdater for an existing OSSA value, because the updater can
+/// insert unnecessary phis in the middle of the original liverange which breaks up the original
+/// liverange into smaller ones:
+/// ```
+///    %1 = def_of_owned_value
+///    %2 = begin_borrow %1
+///    ...
+///    br bb2(%1)
+///  bb2(%3 : @owned $T): // inserted by SSAUpdater
+///    ...
+///    end_borrow %2      // use after end-of-lifetime!
+///    destroy_value %3
+/// ```
+///
+/// It's not needed to run this utility if SSAUpdater is used to create a _new_ OSSA liverange.
+///
+func replacePhisWithIncomingValues(phis: [Phi], _ context: some MutatingContext) {
+  var currentPhis = phis
+  // Do this in a loop because replacing one phi might open up the opportunity for another phi
+  // and the order of phis in the array can be arbitrary.
+  while true {
+    var newPhis = [Phi]()
+    for phi in currentPhis {
+      if !replacePhiWithIncomingValue(phi: phi, context) {
+        newPhis.append(phi)
+      }
+    }
+    if newPhis.count == currentPhis.count {
+      return
+    }
+    currentPhis = newPhis
+  }
+}
+
+func registerPhiUpdater() {
+  BridgedUtilities.registerPhiUpdater(
+    // updateAllGuaranteedPhis
     { (bridgedCtxt: BridgedPassContext, bridgedFunction: BridgedFunction) in
       let context = FunctionPassContext(_bridged: bridgedCtxt)
       let function = bridgedFunction.function;
       updateGuaranteedPhis(in: function, context)
     },
+    // updateGuaranteedPhis
     { (bridgedCtxt: BridgedPassContext, bridgedPhiArray: BridgedArrayRef) in
       let context = FunctionPassContext(_bridged: bridgedCtxt)
       var guaranteedPhis = Stack<Phi>(context)
@@ -172,6 +259,15 @@ func registerGuaranteedPhiUpdater() {
         }
       }
       updateGuaranteedPhis(phis: guaranteedPhis, context)
+    },
+    // replacePhisWithIncomingValues
+    { (bridgedCtxt: BridgedPassContext, bridgedPhiArray: BridgedArrayRef) in
+      let context = FunctionPassContext(_bridged: bridgedCtxt)
+      var phis = [Phi]()
+      bridgedPhiArray.withElements(ofType: BridgedValue.self) {
+        phis = $0.map { Phi($0.value)! }
+      }
+      replacePhisWithIncomingValues(phis: phis, context)
     }
   )
 }
diff --git a/SwiftCompilerSources/Sources/SIL/Operand.swift b/SwiftCompilerSources/Sources/SIL/Operand.swift
@@ -13,7 +13,7 @@
 import SILBridging
 
 /// An operand of an instruction.
-public struct Operand : CustomStringConvertible, NoReflectionChildren {
+public struct Operand : CustomStringConvertible, NoReflectionChildren, Equatable {
   public let bridged: BridgedOperand
 
   public init(bridged: BridgedOperand) {
diff --git a/include/swift/SILOptimizer/OptimizerBridging.h b/include/swift/SILOptimizer/OptimizerBridging.h
@@ -134,8 +134,9 @@ struct BridgedUtilities {
   typedef void (* _Nonnull UpdatePhisFn)(BridgedPassContext, BridgedArrayRef);
 
   static void registerVerifier(VerifyFunctionFn verifyFunctionFn);
-  static void registerGuaranteedPhiUpdater(UpdateFunctionFn updateBorrowedFromFn,
-                                           UpdatePhisFn updateBorrowedFromPhisFn);
+  static void registerPhiUpdater(UpdateFunctionFn updateBorrowedFromFn,
+                                 UpdatePhisFn updateBorrowedFromPhisFn,
+                                 UpdatePhisFn replacePhisWithIncomingValuesFn);
 };
 
 struct BridgedBasicBlockSet {
diff --git a/include/swift/SILOptimizer/Utils/OwnershipOptUtils.h b/include/swift/SILOptimizer/Utils/OwnershipOptUtils.h
@@ -364,6 +364,9 @@ void updateAllGuaranteedPhis(SILPassManager *pm, SILFunction *f);
 /// Updates the reborrow flags and the borrowed-from instructions for all `phis`.
 void updateGuaranteedPhis(SILPassManager *pm, ArrayRef<SILPhiArgument *> phis);
 
+/// Replaces phis with the unique incoming values if all incoming values are the same.
+void replacePhisWithIncomingValues(SILPassManager *pm, ArrayRef<SILPhiArgument *> phis);
+
 } // namespace swift
 
 #endif
diff --git a/include/swift/SILOptimizer/Utils/SILSSAUpdater.h b/include/swift/SILOptimizer/Utils/SILSSAUpdater.h
@@ -35,10 +35,6 @@ class SILBasicBlock;
 class SILType;
 class SILUndef;
 
-/// Independent utility that canonicalizes BB arguments by reusing structurally
-/// equivalent arguments and replacing the original arguments with casts.
-SILValue replaceBBArgWithCast(SILPhiArgument *arg);
-
 /// This class updates SSA for a set of SIL instructions defined in multiple
 /// blocks.
 class SILSSAUpdater {
diff --git a/lib/SILOptimizer/LoopTransforms/LoopRotate.cpp b/lib/SILOptimizer/LoopTransforms/LoopRotate.cpp
@@ -46,7 +46,7 @@ static llvm::cl::opt<bool> RotateSingleBlockLoop("looprotate-single-block-loop",
 
 static bool rotateLoop(SILLoop *loop, DominanceInfo *domInfo,
                        SILLoopInfo *loopInfo, bool rotateSingleBlockLoops,
-                       SILBasicBlock *upToBB);
+                       SILBasicBlock *upToBB, SILPassManager *pm);
 
 /// Check whether all operands are loop invariant.
 static bool
@@ -151,7 +151,8 @@ static void mapOperands(SILInstruction *inst,
 static void updateSSAForUseOfValue(
     SILSSAUpdater &updater, SmallVectorImpl<SILPhiArgument *> &insertedPhis,
     const llvm::DenseMap<ValueBase *, SILValue> &valueMap,
-    SILBasicBlock *Header, SILBasicBlock *EntryCheckBlock, SILValue Res) {
+    SILBasicBlock *Header, SILBasicBlock *EntryCheckBlock, SILValue Res,
+    SILPassManager *pm) {
   // Find the mapped instruction.
   assert(valueMap.count(Res) && "Expected to find value in map!");
   SILValue MappedValue = valueMap.find(Res)->second;
@@ -187,34 +188,25 @@ static void updateSSAForUseOfValue(
     updater.rewriteUse(*use);
   }
 
-  // Canonicalize inserted phis to avoid extra BB Args and if we find an address
-  // phi, stash it so we can handle it after we are done rewriting.
-  for (SILPhiArgument *arg : insertedPhis) {
-    if (SILValue inst = replaceBBArgWithCast(arg)) {
-      arg->replaceAllUsesWith(inst);
-      // DCE+SimplifyCFG runs as a post-pass cleanup.
-      // DCE replaces dead arg values with undef.
-      // SimplifyCFG deletes the dead BB arg.
-      continue;
-    }
-  }
+  replacePhisWithIncomingValues(pm, insertedPhis);
 }
 
 static void
 updateSSAForUseOfInst(SILSSAUpdater &updater,
                       SmallVectorImpl<SILPhiArgument *> &insertedPhis,
                       const llvm::DenseMap<ValueBase *, SILValue> &valueMap,
                       SILBasicBlock *header, SILBasicBlock *entryCheckBlock,
-                      SILInstruction *inst) {
+                      SILInstruction *inst, SILPassManager *pm) {
   for (auto result : inst->getResults())
     updateSSAForUseOfValue(updater, insertedPhis, valueMap, header,
-                           entryCheckBlock, result);
+                           entryCheckBlock, result, pm);
 }
 
 /// Rewrite the code we just created in the preheader and update SSA form.
 static void rewriteNewLoopEntryCheckBlock(
     SILBasicBlock *header, SILBasicBlock *entryCheckBlock,
-    const llvm::DenseMap<ValueBase *, SILValue> &valueMap) {
+    const llvm::DenseMap<ValueBase *, SILValue> &valueMap,
+    SILPassManager *pm) {
   SmallVector<SILPhiArgument *, 8> insertedPhis;
   SILSSAUpdater updater(&insertedPhis);
 
@@ -223,7 +215,7 @@ static void rewriteNewLoopEntryCheckBlock(
   for (unsigned i : range(header->getNumArguments())) {
     auto *arg = header->getArguments()[i];
     updateSSAForUseOfValue(updater, insertedPhis, valueMap, header,
-                           entryCheckBlock, arg);
+                           entryCheckBlock, arg, pm);
   }
 
   auto instIter = header->begin();
@@ -232,7 +224,7 @@ static void rewriteNewLoopEntryCheckBlock(
   while (instIter != header->end()) {
     auto &inst = *instIter;
     updateSSAForUseOfInst(updater, insertedPhis, valueMap, header,
-                          entryCheckBlock, &inst);
+                          entryCheckBlock, &inst, pm);
     ++instIter;
   }
 }
@@ -254,7 +246,7 @@ static void updateDomTree(DominanceInfo *domInfo, SILBasicBlock *preheader,
 }
 
 static bool rotateLoopAtMostUpToLatch(SILLoop *loop, DominanceInfo *domInfo,
-                                      SILLoopInfo *loopInfo) {
+                                      SILLoopInfo *loopInfo, SILPassManager *pm) {
   auto *latch = loop->getLoopLatch();
   if (!latch) {
     LLVM_DEBUG(llvm::dbgs()
@@ -264,11 +256,11 @@ static bool rotateLoopAtMostUpToLatch(SILLoop *loop, DominanceInfo *domInfo,
 
   bool didRotate = rotateLoop(
       loop, domInfo, loopInfo,
-      RotateSingleBlockLoop /* rotateSingleBlockLoops */, latch);
+      RotateSingleBlockLoop /* rotateSingleBlockLoops */, latch, pm);
 
   // Keep rotating at most until we hit the original latch.
   if (didRotate)
-    while (rotateLoop(loop, domInfo, loopInfo, false, latch)) {
+    while (rotateLoop(loop, domInfo, loopInfo, false, latch, pm)) {
     }
 
   return didRotate;
@@ -317,7 +309,7 @@ static bool isSingleBlockLoop(SILLoop *L) {
 /// loop for termination.
 static bool rotateLoop(SILLoop *loop, DominanceInfo *domInfo,
                        SILLoopInfo *loopInfo, bool rotateSingleBlockLoops,
-                       SILBasicBlock *upToBB) {
+                       SILBasicBlock *upToBB, SILPassManager *pm) {
   assert(loop != nullptr && domInfo != nullptr && loopInfo != nullptr
          && "Missing loop information");
 
@@ -443,7 +435,7 @@ static bool rotateLoop(SILLoop *loop, DominanceInfo *domInfo,
 
   // If there were any uses of instructions in the duplicated loop entry check
   // block rewrite them using the ssa updater.
-  rewriteNewLoopEntryCheckBlock(header, preheader, valueMap);
+  rewriteNewLoopEntryCheckBlock(header, preheader, valueMap, pm);
 
   loop->moveToHeader(newHeader);
 
@@ -509,7 +501,7 @@ class LoopRotation : public SILFunctionTransform {
         SILLoop *loop = worklist.pop_back_val();
         changed |= canonicalizeLoop(loop, domInfo, loopInfo);
         changed |=
-            rotateLoopAtMostUpToLatch(loop, domInfo, loopInfo);
+            rotateLoopAtMostUpToLatch(loop, domInfo, loopInfo, getPassManager());
       }
     }
 
diff --git a/lib/SILOptimizer/Utils/OwnershipOptUtils.cpp b/lib/SILOptimizer/Utils/OwnershipOptUtils.cpp
@@ -1915,11 +1915,14 @@ bool swift::extendStoreBorrow(StoreBorrowInst *sbi,
 
 static BridgedUtilities::UpdateFunctionFn updateAllGuaranteedPhisFunction;
 static BridgedUtilities::UpdatePhisFn updateGuaranteedPhisFunction;
+static BridgedUtilities::UpdatePhisFn replacePhisWithIncomingValuesFunction;
 
-void BridgedUtilities::registerGuaranteedPhiUpdater(UpdateFunctionFn updateAllGuaranteedPhisFn,
-                                                    UpdatePhisFn updateGuaranteedPhisFn) {
+void BridgedUtilities::registerPhiUpdater(UpdateFunctionFn updateAllGuaranteedPhisFn,
+                                          UpdatePhisFn updateGuaranteedPhisFn,
+                                          UpdatePhisFn replacePhisWithIncomingValuesFn) {
   updateAllGuaranteedPhisFunction = updateAllGuaranteedPhisFn;
   updateGuaranteedPhisFunction = updateGuaranteedPhisFn;
+  replacePhisWithIncomingValuesFunction = replacePhisWithIncomingValuesFn;
 }
 
 void swift::updateAllGuaranteedPhis(SILPassManager *pm, SILFunction *f) {
@@ -1937,3 +1940,14 @@ void swift::updateGuaranteedPhis(SILPassManager *pm, ArrayRef<SILPhiArgument *>
   }
   updateGuaranteedPhisFunction({pm->getSwiftPassInvocation()}, ArrayRef(bridgedPhis));
 }
+
+void swift::replacePhisWithIncomingValues(SILPassManager *pm, ArrayRef<SILPhiArgument *> phis) {
+  if (!replacePhisWithIncomingValuesFunction)
+    return;
+
+  llvm::SmallVector<BridgedValue, 8> bridgedPhis;
+  for (SILPhiArgument *phi : phis) {
+    bridgedPhis.push_back({phi});
+  }
+  replacePhisWithIncomingValuesFunction({pm->getSwiftPassInvocation()}, ArrayRef(bridgedPhis));
+}
diff --git a/lib/SILOptimizer/Utils/SILSSAUpdater.cpp b/lib/SILOptimizer/Utils/SILSSAUpdater.cpp
diff --git a/test/SILOptimizer/looprotate.sil b/test/SILOptimizer/looprotate.sil
diff --git a/test/SILOptimizer/looprotate_ossa.sil b/test/SILOptimizer/looprotate_ossa.sil

Original file line number	Diff line number	Diff line change
`@@ -140,5 +140,5 @@ private func registerSwiftAnalyses() {`
`140`	`140`
`141`	`141`	`private func registerUtilities() {`
`142`	`142`	`registerVerifier()`
`143`		`- registerGuaranteedPhiUpdater()`
	`143`	`+ registerPhiUpdater()`
`144`	`144`	`}`