Enable ArrayBoundsCheckElimination on OSSA

Author: meg-gupta

lib/SILOptimizer/Analysis/ArraySemantic.cpp

@@ -12,6 +12,7 @@
 
 #include "swift/SILOptimizer/Analysis/ArraySemantic.h"
 #include "swift/SIL/DebugUtils.h"
+#include "swift/SIL/InstructionUtils.h"
 #include "swift/SIL/SILArgument.h"
 #include "swift/SIL/SILBuilder.h"
 #include "swift/SIL/SILFunction.h"
@@ -295,6 +296,10 @@ static bool canHoistArrayArgument(ApplyInst *SemanticsCall, SILValue Arr,
   if (DT->dominates(SelfBB, InsertBefore->getParent()))
     return true;
 
+  if (auto *Copy = dyn_cast<CopyValueInst>(SelfVal)) {
+    // look through one level
+    SelfVal = Copy->getOperand();
+  }
   if (auto LI = dyn_cast<LoadInst>(SelfVal)) {
     // Are we loading a value from an address in a struct defined at a point
     // dominating the hoist point.
@@ -354,18 +359,28 @@ bool swift::ArraySemanticsCall::canHoist(SILInstruction *InsertBefore,
   return false;
 }
 
-/// Copy the array load to the insert point.
-static SILValue copyArrayLoad(SILValue ArrayStructValue,
-                               SILInstruction *InsertBefore,
-                               DominanceInfo *DT) {
+/// Copy the array self value to the insert point.
+static SILValue copySelfValue(SILValue ArrayStructValue,
+                              SILInstruction *InsertBefore, DominanceInfo *DT) {
+  auto *func = InsertBefore->getFunction();
   if (DT->dominates(ArrayStructValue->getParentBlock(),
-                    InsertBefore->getParent()))
+                    InsertBefore->getParent())) {
+    assert(!func->hasOwnership() ||
+           ArrayStructValue.getOwnershipKind() == OwnershipKind::Owned ||
+           ArrayStructValue.getOwnershipKind() == OwnershipKind::Guaranteed);
     return ArrayStructValue;
+  }
 
-  auto *LI = cast<LoadInst>(ArrayStructValue);
+  assert(!func->hasOwnership() ||
+         ArrayStructValue.getOwnershipKind() == OwnershipKind::Owned);
 
-  // Recursively move struct_element_addr.
-  ValueBase *Val = LI->getOperand();
+  SILValue Val;
+  if (auto *Load = dyn_cast<LoadInst>(ArrayStructValue)) {
+    Val = Load->getOperand();
+  } else {
+    auto *Copy = cast<CopyValueInst>(ArrayStructValue);
+    Val = cast<LoadInst>(Copy->getOperand())->getOperand();
+  }
   auto *InsertPt = InsertBefore;
   while (!DT->dominates(Val->getParentBlock(), InsertBefore->getParent())) {
     auto *Inst = cast<StructElementAddrInst>(Val);
@@ -374,7 +389,16 @@ static SILValue copyArrayLoad(SILValue ArrayStructValue,
     InsertPt = Inst;
   }
 
-  return cast<LoadInst>(LI->clone(InsertBefore));
+  if (!ArrayStructValue->getFunction()->hasOwnership()) {
+    return cast<LoadInst>(ArrayStructValue)->clone(InsertBefore);
+  }
+  if (auto *Load = dyn_cast<LoadInst>(ArrayStructValue)) {
+    return Load->clone(InsertBefore);
+  }
+  auto *Copy = cast<CopyValueInst>(ArrayStructValue);
+  auto Addr = cast<LoadInst>(Copy->getOperand())->getOperand();
+  return SILBuilderWithScope(InsertPt).createLoad(InsertPt->getLoc(), Addr,
+                                                  LoadOwnershipQualifier::Copy);
 }
 
 static ApplyInst *hoistOrCopyCall(ApplyInst *AI, SILInstruction *InsertBefore,
@@ -404,17 +428,16 @@ static SILValue hoistOrCopySelf(ApplyInst *SemanticsCall,
 
   auto Self = SemanticsCall->getSelfArgument();
   bool IsOwnedSelf = SelfConvention == ParameterConvention::Direct_Owned;
+  auto *Func = SemanticsCall->getFunction();
 
   // Emit matching release for owned self if we are moving the original call.
   if (!LeaveOriginal && IsOwnedSelf) {
     SILBuilderWithScope Builder(SemanticsCall);
-    Builder.createReleaseValue(SemanticsCall->getLoc(), Self, Builder.getDefaultAtomicity());
+    Builder.emitDestroyValueOperation(SemanticsCall->getLoc(), Self);
   }
-
-  auto NewArrayStructValue = copyArrayLoad(Self, InsertBefore, DT);
-
-  // Retain the array.
-  if (IsOwnedSelf) {
+  auto NewArrayStructValue = copySelfValue(Self, InsertBefore, DT);
+  if (!Func->hasOwnership() && IsOwnedSelf) {
+    // Retain the array.
     SILBuilderWithScope Builder(InsertBefore, SemanticsCall);
     Builder.createRetainValue(SemanticsCall->getLoc(), NewArrayStructValue,
                               Builder.getDefaultAtomicity());
@@ -512,8 +535,7 @@ void swift::ArraySemanticsCall::removeCall() {
   if (getSelfParameterConvention(SemanticsCall) ==
       ParameterConvention::Direct_Owned) {
     SILBuilderWithScope Builder(SemanticsCall);
-    Builder.createReleaseValue(SemanticsCall->getLoc(), getSelf(),
-                               Builder.getDefaultAtomicity());
+    Builder.emitDestroyValueOperation(SemanticsCall->getLoc(), getSelf());
   }
 
   switch (getKind()) {

lib/SILOptimizer/LoopTransforms/ArrayBoundsCheckOpts.cpp

@@ -78,7 +78,7 @@ static SILValue getArrayStructPointer(ArrayCallKind K, SILValue Array) {
   assert(K != ArrayCallKind::kNone);
 
   if (K < ArrayCallKind::kMakeMutable) {
-    auto LI = dyn_cast<LoadInst>(Array);
+    auto LI = dyn_cast<LoadInst>(lookThroughCopyValueInsts(Array));
     if (!LI) {
       return Array;
     }
@@ -107,20 +107,15 @@ mayChangeArraySize(SILInstruction *I, ArrayCallKind &Kind, SILValue &Array,
 
   // TODO: What else.
   if (isa<StrongRetainInst>(I) || isa<RetainValueInst>(I) ||
-      isa<CondFailInst>(I) || isa<DeallocStackInst>(I) ||
-      isa<AllocationInst>(I))
+      isa<CopyValueInst>(I) || isa<CondFailInst>(I) ||
+      isa<DeallocStackInst>(I) || isa<AllocationInst>(I))
     return ArrayBoundsEffect::kNone;
 
-  // A retain on an arbitrary class can have sideeffects because of the deinit
+  // A release on an arbitrary class can have sideeffects because of the deinit
   // function.
-  if (auto SR = dyn_cast<StrongReleaseInst>(I))
-    return isReleaseSafeArrayReference(SR->getOperand(),
-                                       ReleaseSafeArrayReferences, RCIA)
-               ? ArrayBoundsEffect::kNone
-               : ArrayBoundsEffect::kMayChangeAny;
-
-  if (auto RV = dyn_cast<ReleaseValueInst>(I))
-    return isReleaseSafeArrayReference(RV->getOperand(),
+  if (isa<StrongReleaseInst>(I) || isa<ReleaseValueInst>(I) ||
+      isa<DestroyValueInst>(I))
+    return isReleaseSafeArrayReference(I->getOperand(0),
                                        ReleaseSafeArrayReferences, RCIA)
                ? ArrayBoundsEffect::kNone
                : ArrayBoundsEffect::kMayChangeAny;
@@ -148,12 +143,22 @@ mayChangeArraySize(SILInstruction *I, ArrayCallKind &Kind, SILValue &Array,
   // A store to an alloc_stack can't possibly store to the array size which is
   // stored in a runtime allocated object sub field of an alloca.
   if (auto *SI = dyn_cast<StoreInst>(I)) {
+    if (SI->getOwnershipQualifier() == StoreOwnershipQualifier::Assign) {
+      // store [assign] can call a destructor with unintended effects
+      return ArrayBoundsEffect::kMayChangeAny;
+    }
     auto Ptr = SI->getDest();
     return isa<AllocStackInst>(Ptr) || isAddressOfArrayElement(SI->getDest())
                ? ArrayBoundsEffect::kNone
                : ArrayBoundsEffect::kMayChangeAny;
   }
 
+  if (isa<LoadInst>(I))
+    return ArrayBoundsEffect::kNone;
+
+  if (isa<BeginBorrowInst>(I) || isa<EndBorrowInst>(I))
+    return ArrayBoundsEffect::kNone;
+
   return ArrayBoundsEffect::kMayChangeAny;
 }
 
@@ -260,7 +265,7 @@ class ABCAnalysis {
         mayChangeArraySize(Inst, K, Array, ReleaseSafeArrayReferences, RCIA);
 
     if (BoundsEffect == ArrayBoundsEffect::kMayChangeAny) {
-      LLVM_DEBUG(llvm::dbgs() << " no safe because kMayChangeAny " << *Inst);
+      LLVM_DEBUG(llvm::dbgs() << " not safe because kMayChangeAny " << *Inst);
       allArraysInMemoryAreUnsafe = true;
       // No need to store specific arrays in this case.
       UnsafeArrays.clear();
@@ -420,7 +425,7 @@ static BuiltinValueKind invertCmpID(BuiltinValueKind ID) {
 }
 
 /// Checks if Start to End is the range of 0 to the count of an array.
-/// Returns the array is this is the case.
+/// Returns the array if this is the case.
 static SILValue getZeroToCountArray(SILValue Start, SILValue End) {
   auto *IL = dyn_cast<IntegerLiteralInst>(Start);
   if (!IL || IL->getValue() != 0)
@@ -433,7 +438,6 @@ static SILValue getZeroToCountArray(SILValue Start, SILValue End) {
   ArraySemanticsCall SemCall(SEI->getOperand());
   if (SemCall.getKind() != ArrayCallKind::kGetCount)
     return SILValue();
-  
   return SemCall.getSelf();
 }
 
@@ -740,9 +744,9 @@ class AccessFunction {
     return nullptr;
   }
 
-  /// Returns true if the loop iterates from 0 until count of \p Array.
-  bool isZeroToCount(SILValue Array) {
-    return getZeroToCountArray(Ind->Start, Ind->End) == Array;
+  /// Returns true if the loop iterates from 0 until count of \p ArrayVal.
+  bool isZeroToCount(SILValue ArrayVal) {
+    return getZeroToCountArray(Ind->Start, Ind->End) == ArrayVal;
   }
 
   /// Hoists the necessary check for beginning and end of the induction
@@ -901,11 +905,6 @@ class ABCOpt : public SILFunctionTransform {
       return;
 
     SILFunction *F = getFunction();
-
-    // FIXME: Update for ownership.
-    if (F->hasOwnership())
-      return;
-
     LI = PM->getAnalysis<SILLoopAnalysis>()->get(F);
     DT = PM->getAnalysis<DominanceAnalysis>()->get(F);
     IVs = PM->getAnalysis<IVAnalysis>()->get(F);
@@ -917,6 +916,8 @@ class ABCOpt : public SILFunctionTransform {
       reportBoundsChecks(F);
     }
 #endif
+    LLVM_DEBUG(llvm::dbgs()
+               << "ArrayBoundsCheckOpts on function: " << F->getName() << "\n");
     // Collect all arrays in this function. A release is only 'safe' if we know
     // its deinitializer does not have sideeffects that could cause memory
     // safety issues. A deinit could deallocate array or put a different array
@@ -931,6 +932,7 @@ class ABCOpt : public SILFunctionTransform {
           // that is not calling a deinit function.
           if (DestAnalysis->mayStoreToMemoryOnDestruction(rcRoot->getType()))
             continue;
+          LLVM_DEBUG(llvm::dbgs() << "ReleaseSafeArray: " << rcRoot << "\n");
           ReleaseSafeArrays.insert(rcRoot);
           ReleaseSafeArrays.insert(
               getArrayStructPointer(ArrayCallKind::kCheckIndex, rcRoot));

test/SILOptimizer/abcopts.sil

@@ -789,7 +789,13 @@ bb0(%0 : $ArraySlice<Int>):
   %t1 = apply %f1(%0) : $@convention(method) (@owned ArraySlice<Int>) -> Int32
   %c1 = struct_extract %t1 : $Int32, #Int32._value
   %t2 = builtin "cmp_eq_Int32"(%z0 : $Builtin.Int32, %c1 : $Builtin.Int32) : $Builtin.Int1
-  cond_br %t2, bb2, bb1(%z0 : $Builtin.Int32)
+  cond_br %t2, bb0a, bb0b
+
+bb0a:
+  br bb2
+
+bb0b:
+ br bb1(%z0 : $Builtin.Int32)
 
 bb1(%i0 : $Builtin.Int32):
   %f2 = function_ref @checkbounds3 : $@convention(method) (Int32, Bool, @owned ArraySlice<Int>) -> _DependenceToken
@@ -805,7 +811,13 @@ bb1(%i0 : $Builtin.Int32):
   %t6 = builtin "sadd_with_overflow_Int32"(%i0 : $Builtin.Int32, %i2 : $Builtin.Int32, %t5 : $Builtin.Int1) : $(Builtin.Int32, Builtin.Int1)
   %t7 = tuple_extract %t6 : $(Builtin.Int32, Builtin.Int1), 0
   %8 = builtin "cmp_eq_Int32"(%t7 : $Builtin.Int32, %c1 : $Builtin.Int32) : $Builtin.Int1
-  cond_br %8, bb2, bb1(%t7 : $Builtin.Int32)
+  cond_br %8, bb1a, bb1b
+
+bb1a:
+  br bb2
+
+bb1b:
+  br bb1(%t7 : $Builtin.Int32)
 
 bb2:
   %r1 = tuple ()