Merge pull request #33431 from atrick/fix-licm-condfail

Fix LICM to avoid hoisting never-executed traps

Author: atrick

include/swift/SIL/LoopInfo.h

@@ -50,6 +50,16 @@ class SILLoop : public llvm::LoopBase<SILBasicBlock, SILLoop> {
   /// this loop by unrolling or versioning.
   bool canDuplicate(SILInstruction *Inst) const;
 
+  void getExitingAndLatchBlocks(
+    SmallVectorImpl<SILBasicBlock *> &ExitingAndLatchBlocks) const {
+    this->getExitingBlocks(ExitingAndLatchBlocks);
+    SILBasicBlock *header = getHeader();
+    for (auto *predBB : header->getPredecessorBlocks()) {
+      if (contains(predBB) && !this->isLoopExiting(predBB))
+        ExitingAndLatchBlocks.push_back(predBB);
+    }
+  }
+
 private:
   friend class llvm::LoopInfoBase<SILBasicBlock, SILLoop>;
 

lib/SILOptimizer/LoopTransforms/LICM.cpp

@@ -214,16 +214,16 @@ static void getDominatingBlocks(SmallVectorImpl<SILBasicBlock *> &domBlocks,
                                 SILLoop *Loop, DominanceInfo *DT) {
   auto HeaderBB = Loop->getHeader();
   auto DTRoot = DT->getNode(HeaderBB);
-  SmallVector<SILBasicBlock *, 8> ExitingBBs;
-  Loop->getExitingBlocks(ExitingBBs);
+  SmallVector<SILBasicBlock *, 8> ExitingAndLatchBBs;
+  Loop->getExitingAndLatchBlocks(ExitingAndLatchBBs);
   for (llvm::df_iterator<DominanceInfoNode *> It = llvm::df_begin(DTRoot),
                                               E = llvm::df_end(DTRoot);
        It != E;) {
     auto *CurBB = It->getBlock();
 
     // Don't decent into control-dependent code. Only traverse into basic blocks
     // that dominate all exits.
-    if (!std::all_of(ExitingBBs.begin(), ExitingBBs.end(),
+    if (!std::all_of(ExitingAndLatchBBs.begin(), ExitingAndLatchBBs.end(),
                      [=](SILBasicBlock *ExitBB) {
           return DT->dominates(CurBB, ExitBB);
         })) {

test/SILOptimizer/licm.sil

@@ -854,3 +854,82 @@ bb9(%39 : $Builtin.Int64):                        // Preds: bb7
   dealloc_stack %3 : $*Index           // id: %41
   return %40 : $Int64                               // id: %42
 }
+
+// testConditionalTrapInInfiniteSyncLoop and
+// testConditionalTrapDominatingSyncLoopExit
+//
+// It's legal for the optimizer to consider code after the loop as
+// always reachable, but when a loop has no exits, or when the loops
+// exits are dominated by a conditional statement we should not
+// consider conditional statements within the loop as dominating all
+// possible execution paths through the loop. At least not when there
+// is at least one path through the loop that contains a
+// "synchronization point", such as a function that may contain a
+// memory barrier, perform I/O, or exit the program.
+sil @mayExit : $@convention(thin) () -> ()
+
+// CHECK-LABEL: sil @testConditionalTrapInInfiniteSyncLoop : $@convention(thin) (Builtin.Int1, Builtin.Int1) -> () {
+// CHECK: bb0
+// CHECK-NOT: cond_fail
+// CHECK:   br bb1
+// CHECK: bb1:
+// CHECK:   cond_br %0, bb2, bb3
+// CHECK: bb2:
+// CHECK: cond_fail %1 : $Builtin.Int1, "arithmetic overflow"
+// CHECK-LABEL: } // end sil function 'testConditionalTrapInInfiniteSyncLoop'
+sil @testConditionalTrapInInfiniteSyncLoop : $@convention(thin) (Builtin.Int1, Builtin.Int1) -> () {
+bb0(%0 : $Builtin.Int1, %1 : $Builtin.Int1):
+  br bb1
+
+bb1: // loop head
+  cond_br %0, bb2, bb3
+
+bb2: // maybe never executed
+  cond_fail %1 : $Builtin.Int1, "arithmetic overflow"
+  br bb4
+
+bb3:
+  // synchronization point: has "real" side-effects that we can't
+  // reorder with traps
+  %f = function_ref @mayExit : $@convention(thin) () -> ()
+  apply %f() : $@convention(thin) () -> ()
+  br bb4
+
+bb4: // latch
+  br bb1
+}
+
+// CHECK-LABEL: sil @testConditionalTrapDominatingSyncLoopExit : $@convention(thin) (Builtin.Int1, Builtin.Int1, Builtin.Int1) -> () {
+// CHECK: bb0
+// CHECK-NOT: cond_fail
+// CHECK:   br bb1
+// CHECK: bb1:
+// CHECK:   cond_br %0, bb2, bb4
+// CHECK: bb2:
+// CHECK:   cond_fail %1 : $Builtin.Int1, "arithmetic overflow"
+// CHECK-LABEL: } // end sil function 'testConditionalTrapDominatingSyncLoopExit'
+sil @testConditionalTrapDominatingSyncLoopExit : $@convention(thin) (Builtin.Int1, Builtin.Int1, Builtin.Int1) -> () {
+bb0(%0 : $Builtin.Int1, %1 : $Builtin.Int1, %2 : $Builtin.Int1):
+  br bb1
+
+bb1: // loop head
+  cond_br %0, bb2, bb4
+
+bb2: // maybe never executed
+  cond_fail %1 : $Builtin.Int1, "arithmetic overflow"
+  cond_br %2, bb3, bb5
+
+bb3: // tail
+  br bb1
+
+bb4:
+  // synchronization point: has "real" side-effects that we can't
+  // reorder with traps
+  %f = function_ref @mayExit : $@convention(thin) () -> ()
+  apply %f() : $@convention(thin) () -> ()
+  br bb1
+
+bb5:
+  %99 = tuple ()
+  return %99 : $()
+}