Merge pull request #61724 from meg-gupta/revertaddrphi

Revert changes to ArrayPropertyOpt to avoid address phis

Author: meg-gupta

lib/SIL/Verifier/SILVerifier.cpp

@@ -985,6 +985,24 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
     return InstNumbers[a] < InstNumbers[b];
   }
 
+  // FIXME: For sanity, address-type phis should be prohibited at all SIL
+  // stages. However, the optimizer currently breaks the invariant in three
+  // places:
+  // 1. Normal Simplify CFG during conditional branch simplification
+  //    (sneaky jump threading).
+  // 2. Simplify CFG via Jump Threading.
+  // 3. Loop Rotation.
+  //
+  // BasicBlockCloner::canCloneInstruction and sinkAddressProjections is
+  // designed to avoid this issue, we just need to make sure all passes use it
+  // correctly.
+  //
+  // Minimally, we must prevent address-type phis as long as access markers are
+  // preserved. A goal is to preserve access markers in OSSA.
+  bool prohibitAddressPhis() {
+    return F.hasOwnership();
+  }
+
   void visitSILPhiArgument(SILPhiArgument *arg) {
     // Verify that the `isPhiArgument` property is sound:
     // - Phi arguments come from branches.
@@ -1008,7 +1026,7 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                 "All phi argument inputs must be from branches.");
       }
     }
-    if (arg->isPhi()) {
+    if (arg->isPhi() && prohibitAddressPhis()) {
       // As a property of well-formed SIL, we disallow address-type
       // phis. Supporting them would prevent reliably reasoning about the
       // underlying storage of memory access. This reasoning is important for

lib/SILOptimizer/LoopTransforms/ArrayPropertyOpt.cpp

@@ -53,19 +53,18 @@
 #define DEBUG_TYPE "array-property-opt"
 
 #include "ArrayOpt.h"
-#include "swift/SIL/BasicBlockBits.h"
-#include "swift/SIL/CFG.h"
-#include "swift/SIL/DebugUtils.h"
-#include "swift/SIL/InstructionUtils.h"
-#include "swift/SIL/LoopInfo.h"
-#include "swift/SIL/Projection.h"
-#include "swift/SIL/SILCloner.h"
 #include "swift/SILOptimizer/Analysis/ArraySemantic.h"
 #include "swift/SILOptimizer/Analysis/LoopAnalysis.h"
 #include "swift/SILOptimizer/PassManager/Transforms.h"
-#include "swift/SILOptimizer/Utils/BasicBlockOptUtils.h"
 #include "swift/SILOptimizer/Utils/CFGOptUtils.h"
 #include "swift/SILOptimizer/Utils/SILSSAUpdater.h"
+#include "swift/SIL/CFG.h"
+#include "swift/SIL/DebugUtils.h"
+#include "swift/SIL/InstructionUtils.h"
+#include "swift/SIL/Projection.h"
+#include "swift/SIL/LoopInfo.h"
+#include "swift/SIL/BasicBlockBits.h"
+#include "swift/SIL/SILCloner.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
@@ -86,8 +85,6 @@ class ArrayPropertiesAnalysis {
   SILBasicBlock *Preheader;
   DominanceInfo *DomTree;
 
-  SinkAddressProjections sinkProj;
-
   llvm::DenseMap<SILFunction *, uint32_t> InstCountCache;
   llvm::SmallSet<SILValue, 16> HoistableArray;
 
@@ -171,18 +168,13 @@ class ArrayPropertiesAnalysis {
 
     bool FoundHoistable = false;
     uint32_t LoopInstCount = 0;
-
     for (auto *BB : Loop->getBlocks()) {
       for (auto &Inst : *BB) {
         // Can't clone alloc_stack instructions whose dealloc_stack is outside
         // the loop.
         if (!Loop->canDuplicate(&Inst))
           return false;
 
-        if (!sinkProj.analyzeAddressProjections(&Inst)) {
-          return false;
-        }
-
         ArraySemanticsCall ArrayPropsInst(&Inst, "array.props", true);
         if (!ArrayPropsInst)
           continue;
@@ -544,15 +536,10 @@ class RegionCloner : public SILCloner<RegionCloner> {
       for (auto *arg : origBB->getArguments())
         updateSSAForValue(origBB, arg, SSAUp);
 
-      SinkAddressProjections sinkProj;
       // Update outside used instruction values.
       for (auto &inst : *origBB) {
-        for (auto result : inst.getResults()) {
-          bool success = sinkProj.analyzeAddressProjections(&inst);
-          assert(success);
-          sinkProj.cloneProjections();
+        for (auto result : inst.getResults())
           updateSSAForValue(origBB, result, SSAUp);
-        }
       }
     }
   }

test/SILOptimizer/array_property_opt.sil

@@ -258,44 +258,3 @@ bb10: // Exit dominated by bb3
 bb11: // Non-exit dominated by bb1
   return %4 : $Builtin.Int1
 }
-
-class Klass {
-}
-
-struct WrapperStruct {
-  var val: Klass
-}
-
-sil @use_klass : $@convention(thin) (@in_guaranteed Klass) -> ()
-
-sil @test_sink_address_proj : $@convention(thin) (@inout MyArray<MyClass>, @in_guaranteed WrapperStruct) -> () {
-bb0(%0 : $*MyArray<MyClass>, %1 : $*WrapperStruct):
-  %3 = load %0 : $*MyArray<MyClass>
-  br bb1
-
-bb1:
-  %2 = function_ref @arrayPropertyIsNative : $@convention(method) (@owned MyArray<MyClass>) -> Bool
-  retain_value %3 : $MyArray<MyClass>
-  %5 = apply %2(%3) : $@convention(method) (@owned MyArray<MyClass>) -> Bool
-  %ele = struct_element_addr %1 : $*WrapperStruct, #WrapperStruct.val
-  cond_br undef, bb5, bb2
-
-bb2:
- %6 = integer_literal $Builtin.Int1, -1
- cond_br %6, bb3, bb4
-
-bb3:
-  br bb1
-
-bb4:
- br bb6
-
-bb5:
- %f = function_ref @use_klass : $@convention(thin) (@in_guaranteed Klass) -> ()
- %a = apply %f(%ele) : $@convention(thin) (@in_guaranteed Klass) -> ()
- br bb6
-
-bb6:
- %t = tuple ()
- return %t : $()
-}