Revert "IRGen: Peephole improvements for enum switch codegen."

This may be the cause of a crash and burn on the iOS builders:

Assertion failed: (Offset <= PieceOffset && "overlapping or duplicate pieces"), function finalize

Swift SVN r29027

lib/IRGen/EnumPayload.cpp

@@ -357,7 +357,7 @@ static void emitSubSwitch(IRGenFunction &IGF,
                     MutableArrayRef<EnumPayload::LazyValue> values,
                     APInt mask,
                     MutableArrayRef<std::pair<APInt, llvm::BasicBlock *>> cases,
-                    SwitchDefaultDest dflt) {
+                    llvm::BasicBlock *dflt) {
 recur:
   assert(!values.empty() && "didn't exit out when exhausting all values?!");
   
@@ -432,28 +432,21 @@ recur:
   // If there's only one case, do a cond_br.
   if (subCases.size() == 1) {
     auto &subCase = *subCases.begin();
-    llvm::BasicBlock *block = blockForCases(subCase.second);
-    // If the default case is unreachable, we don't need to conditionally
-    // branch.
-    if (dflt.getInt()) {
-      IGF.Builder.CreateBr(block);
-      goto next;
-    }
-  
     auto &valuePiece = subCase.first;
     llvm::Value *valueConstant = llvm::ConstantInt::get(payloadIntTy,
                                                         valuePiece);
     valueConstant = IGF.Builder.CreateBitOrPointerCast(valueConstant,
                                                        v->getType());
     auto cmp = IGF.Builder.CreateICmpEQ(v, valueConstant);
-    IGF.Builder.CreateCondBr(cmp, block, dflt.getPointer());
+    llvm::BasicBlock *block = blockForCases(subCase.second);
+    IGF.Builder.CreateCondBr(cmp, block, dflt);
     goto next;
   }
   
   // Otherwise, do a switch.
   {
     v = IGF.Builder.CreateBitOrPointerCast(v, payloadIntTy);
-    auto swi = IGF.Builder.CreateSwitch(v, dflt.getPointer(), subCases.size());
+    auto swi = IGF.Builder.CreateSwitch(v, dflt, subCases.size());
     
     for (auto &subCase : subCases) {
       auto &valuePiece = subCase.first;
@@ -475,17 +468,11 @@ next:
 void EnumPayload::emitSwitch(IRGenFunction &IGF,
                            APInt mask,
                            ArrayRef<std::pair<APInt, llvm::BasicBlock *>> cases,
-                           SwitchDefaultDest dflt) const {
+                           llvm::BasicBlock *dflt) const {
   // If there's only one case to test, do a simple compare and branch.
   if (cases.size() == 1) {
-    // If the default case is unreachable, don't bother branching at all.
-    if (dflt.getInt()) {
-      IGF.Builder.CreateBr(cases[0].second);
-      return;
-    }
-  
     auto *cmp = emitCompare(IGF, mask, cases[0].first);
-    IGF.Builder.CreateCondBr(cmp, cases[0].second, dflt.getPointer());
+    IGF.Builder.CreateCondBr(cmp, cases[0].second, dflt);
     return;
   }
 

lib/IRGen/EnumPayload.h

@@ -80,15 +80,6 @@ public:
   }
 };
 
-/// Is a switch default destination unreachable?
-enum IsUnreachable_t: bool {
-  IsNotUnreachable = false,
-  IsUnreachable = true,
-};
-
-using SwitchDefaultDest
-  = llvm::PointerIntPair<llvm::BasicBlock*, 1, IsUnreachable_t>;
-
 /// An enum payload value. The payload is represented as an explosion of
 /// integers and pointers that together represent the bit pattern of
 /// the payload.
@@ -152,7 +143,7 @@ public:
   void emitSwitch(IRGenFunction &IGF,
                   APInt mask,
                   ArrayRef<std::pair<APInt, llvm::BasicBlock*>> cases,
-                  SwitchDefaultDest dflt) const;
+                  llvm::BasicBlock *dflt) const;
   
   /// Emit an equality comparison operation that payload & mask == value.
   llvm::Value *emitCompare(IRGenFunction &IGF,

lib/IRGen/GenEnum.cpp

@@ -1393,6 +1393,37 @@ namespace {
       llvm::BasicBlock *payloadDest = blockForCase(getPayloadElement());
       unsigned extraInhabitantCount = getNumExtraInhabitantTagValues();
 
+      // If there are extra tag bits, switch over them first.
+      SmallVector<llvm::BasicBlock*, 2> tagBitBlocks;
+      if (ExtraTagBitCount > 0) {
+        llvm::Value *tagBits = value.claimNext();
+
+        auto *swi = IGF.Builder.CreateSwitch(tagBits, unreachableBB,
+                                             NumExtraTagValues);
+
+        // If we have extra inhabitants, we need to check for them in the
+        // zero-tag case. Otherwise, we switch directly to the payload case.
+        if (extraInhabitantCount > 0) {
+          auto bb = llvm::BasicBlock::Create(C);
+          tagBitBlocks.push_back(bb);
+          swi->addCase(llvm::ConstantInt::get(C,APInt(ExtraTagBitCount,0)), bb);
+        } else {
+          tagBitBlocks.push_back(payloadDest);
+          swi->addCase(llvm::ConstantInt::get(C,APInt(ExtraTagBitCount,0)),
+                       payloadDest);
+        }
+
+        for (unsigned i = 1; i < NumExtraTagValues; ++i) {
+          auto bb = llvm::BasicBlock::Create(C);
+          tagBitBlocks.push_back(bb);
+          swi->addCase(llvm::ConstantInt::get(C,APInt(ExtraTagBitCount,i)), bb);
+        }
+
+        // Continue by emitting the extra inhabitant dispatch, if any.
+        if (extraInhabitantCount > 0)
+          IGF.Builder.emitBlock(tagBitBlocks[0]);
+      }
+
       auto elements = getPayloadElement()->getParentEnum()->getAllElements();
       auto elti = elements.begin(), eltEnd = elements.end();
       if (*elti == getPayloadElement())
@@ -1408,72 +1439,11 @@ namespace {
         return result;
       };
 
-      // If there are extra tag bits, switch over them first.
-      SmallVector<llvm::BasicBlock*, 2> tagBitBlocks;
-      if (ExtraTagBitCount > 0) {
-        llvm::Value *tagBits = value.claimNext();
-        assert(NumExtraTagValues > 1
-               && "should have more than two tag values if there are extra "
-                  "tag bits!");
-
-        llvm::BasicBlock *zeroDest;
-        // If we have extra inhabitants, we need to check for them in the
-        // zero-tag case. Otherwise, we switch directly to the payload case.
-        if (extraInhabitantCount > 0)
-          zeroDest = llvm::BasicBlock::Create(C);
-        else
-          zeroDest = payloadDest;
-
-        // If there are only two interesting cases, do a cond_br instead of
-        // a switch.
-        if (ExtraTagBitCount == 1) {
-          tagBitBlocks.push_back(zeroDest);
-          llvm::BasicBlock *oneDest;
-          
-          // If there's only one no-payload case, we can jump to it directly.
-          if (ElementsWithNoPayload.size() == 1) {
-            oneDest = blockForCase(nextCase());
-          } else {
-            oneDest = llvm::BasicBlock::Create(C);
-            tagBitBlocks.push_back(oneDest);
-          }
-          IGF.Builder.CreateCondBr(tagBits, oneDest, zeroDest);
-        } else {
-          auto *swi = IGF.Builder.CreateSwitch(tagBits, unreachableBB,
-                                               NumExtraTagValues);
-
-          // If we have extra inhabitants, we need to check for them in the
-          // zero-tag case. Otherwise, we switch directly to the payload case.
-          tagBitBlocks.push_back(zeroDest);
-          swi->addCase(llvm::ConstantInt::get(C,APInt(ExtraTagBitCount,0)),
-                       zeroDest);
-          
-          for (unsigned i = 1; i < NumExtraTagValues; ++i) {
-            // If there's only one no-payload case, or the payload is empty,
-            // we can jump directly to cases without more branching.
-            llvm::BasicBlock *bb;
-            
-            if (ElementsWithNoPayload.size() == 1
-                || PayloadBitCount == 0) {
-              bb = blockForCase(nextCase());
-            } else {
-              bb = llvm::BasicBlock::Create(C);
-              tagBitBlocks.push_back(bb);
-            }
-            swi->addCase(llvm::ConstantInt::get(C, APInt(ExtraTagBitCount, i)),
-                         bb);
-          }
-
-          // Continue by emitting the extra inhabitant dispatch, if any.
-          if (extraInhabitantCount > 0)
-            IGF.Builder.emitBlock(tagBitBlocks[0]);
-        }
-      }
-
       // If there are no extra tag bits, or they're set to zero, then we either
       // have a payload, or an empty case represented using an extra inhabitant.
       // Check the extra inhabitant cases if we have any.
       auto &fpTypeInfo = getFixedPayloadTypeInfo();
+      unsigned payloadBits = fpTypeInfo.getFixedSize().getValueInBits();
       if (extraInhabitantCount > 0) {
         // Switch over the extra inhabitant patterns we used.
         APInt mask = fpTypeInfo.getFixedExtraInhabitantMask(IGF.IGM);
@@ -1481,13 +1451,12 @@ namespace {
         SmallVector<std::pair<APInt, llvm::BasicBlock *>, 4> cases;
         for (auto i = 0U; i < extraInhabitantCount && elti != eltEnd; ++i) {
           cases.push_back({
-            fpTypeInfo.getFixedExtraInhabitantValue(IGF.IGM, PayloadBitCount,i),
+            fpTypeInfo.getFixedExtraInhabitantValue(IGF.IGM, payloadBits, i),
             blockForCase(nextCase())
           });
         }
         
-        payload.emitSwitch(IGF, mask, cases,
+        payload.emitSwitch(IGF, mask, cases, payloadDest);
-                           SwitchDefaultDest(payloadDest, IsNotUnreachable));
       }
 
       // We should have handled the payload case either in extra inhabitant
@@ -1495,12 +1464,19 @@ namespace {
       assert(IGF.Builder.hasPostTerminatorIP() &&
              "did not handle payload case");
 
+      // If there's an empty payload, each tag value corresponds to a single
+      // empty case.
+      // TODO: Skip the waypoint blocks here.
+      if (payloadBits == 0) {
+        for (unsigned i = 1, e = tagBitBlocks.size(); i < e; ++i) {
+          assert(elti != eltEnd &&
+                 "ran out of cases before running out of extra tags?");
+          IGF.Builder.emitBlock(tagBitBlocks[i]);
+          IGF.Builder.CreateBr(blockForCase(nextCase()));
+        }
+      } else {
         // Handle the cases covered by each tag bit value.
-      // If there was only one no-payload case, or the payload is empty, we
+        unsigned casesPerTag = payloadBits >= 32 ? UINT_MAX : 1U << payloadBits;
-      // already branched in the first switch.
-      if (PayloadBitCount > 0 && ElementsWithNoPayload.size() > 1) {
-        unsigned casesPerTag = PayloadBitCount >= 32
-          ? UINT_MAX : 1U << PayloadBitCount;
         for (unsigned i = 1, e = tagBitBlocks.size(); i < e; ++i) {
           assert(elti != eltEnd &&
                  "ran out of cases before running out of extra tags?");
@@ -1508,20 +1484,16 @@ namespace {
           
           SmallVector<std::pair<APInt, llvm::BasicBlock *>, 4> cases;
           for (unsigned tag = 0; tag < casesPerTag && elti != eltEnd; ++tag) {
-            cases.push_back({APInt(PayloadBitCount, tag),
+            cases.push_back({APInt(payloadBits, tag),
                              blockForCase(nextCase())});
           }
           
-          // FIXME: Provide a mask to only match the bits in the payload
+          // FIXME: Provide a mask for 
-          // whose extra inhabitants differ.
+          payload.emitSwitch(IGF, APInt::getAllOnesValue(payloadBits), cases,
-          payload.emitSwitch(IGF, APInt::getAllOnesValue(PayloadBitCount),
+                             unreachableBB);
-                             cases,
-                             SwitchDefaultDest(unreachableBB, IsUnreachable));
         }
       }
 
-      assert(elti == eltEnd && "did not branch to all cases?!");
-
       // Delete the unreachable default block if we didn't use it, or emit it
       // if we did.
       if (unreachableBB->use_empty()) {
@@ -1720,26 +1692,22 @@ namespace {
     testFixedEnumContainsPayload(IRGenFunction &IGF,
                                  const EnumPayload &payload,
                                  llvm::Value *extraBits) const {
-      auto *nonzeroBB = llvm::BasicBlock::Create(IGF.IGM.getLLVMContext());
+      auto *falseBB = llvm::BasicBlock::Create(IGF.IGM.getLLVMContext());
+
       // We only need to apply the payload operation if the enum contains a
       // value of the payload case.
 
       // If we have extra tag bits, they will be zero if we contain a payload.
       if (ExtraTagBitCount > 0) {
         assert(extraBits);
-        llvm::Value *isNonzero;
-        if (ExtraTagBitCount == 1) {
-          isNonzero = extraBits;
-        } else {
         llvm::Value *zero = llvm::ConstantInt::get(extraBits->getType(), 0);
-          isNonzero = IGF.Builder.CreateICmp(llvm::CmpInst::ICMP_NE,
+        llvm::Value *isZero = IGF.Builder.CreateICmp(llvm::CmpInst::ICMP_EQ,
                                                      extraBits, zero);
-        }
 
-        auto *zeroBB = llvm::BasicBlock::Create(IGF.IGM.getLLVMContext());
+        auto *trueBB = llvm::BasicBlock::Create(IGF.IGM.getLLVMContext());
-        IGF.Builder.CreateCondBr(isNonzero, nonzeroBB, zeroBB);
+        IGF.Builder.CreateCondBr(isZero, trueBB, falseBB);
 
-        IGF.Builder.emitBlock(zeroBB);
+        IGF.Builder.emitBlock(trueBB);
       }
 
       // If we used extra inhabitants to represent empty case discriminators,
@@ -1760,17 +1728,16 @@ namespace {
              ++i, ++inhabitant) {
           auto xi = getFixedPayloadTypeInfo()
             .getFixedExtraInhabitantValue(IGF.IGM, bitWidth, inhabitant);
-          cases.push_back({xi, nonzeroBB});
+          cases.push_back({xi, falseBB});
         }
         
         auto mask
           = getFixedPayloadTypeInfo().getFixedExtraInhabitantMask(IGF.IGM);
-        payload.emitSwitch(IGF, mask, cases,
+        payload.emitSwitch(IGF, mask, cases, payloadBB);
-                           SwitchDefaultDest(payloadBB, IsNotUnreachable));
         IGF.Builder.emitBlock(payloadBB);
       }
 
-      return nonzeroBB;
+      return falseBB;
     }
 
     /// Emits the test(s) that determine whether the enum contains a payload
@@ -2949,8 +2916,7 @@ namespace {
         }
 
         parts.payload.emitSwitch(IGF, APInt::getAllOnesValue(PayloadBitCount),
-                               cases,
+                                 cases, unreachableBB);
-                               SwitchDefaultDest(unreachableBB, IsUnreachable));
         ++tagIndex;
       }
 

test/IRGen/enum.sil

@@ -450,9 +450,12 @@ enum SinglePayloadNoXI2 {
 sil @single_payload_no_xi_switch : $@convention(thin) (SinglePayloadNoXI2) -> () {
 // CHECK: entry:
 entry(%u : $SinglePayloadNoXI2):
-// CHECK:   br i1 %1, label %[[TAGS:[0-9]+]], label %[[X_DEST:[0-9]+]]
+// CHECK:   switch i1 %1, label %[[DFLT:[0-9]+]] [
+// CHECK:     i1 false, label %[[X_DEST:[0-9]+]]
+// CHECK:     i1 true, label %[[TAGS:[0-9]+]]
+// CHECK:   ]
 // CHECK: ; <label>:[[TAGS]]
-// CHECK:   switch [[WORD]] %0, label %[[DFLT:[0-9]+]] [
+// CHECK:   switch [[WORD]] %0, label %[[DFLT]] [
 // CHECK:     [[WORD]] 0, label %[[Y_DEST:[0-9]+]]
 // CHECK:     [[WORD]] 1, label %[[Z_DEST:[0-9]+]]
 // CHECK:     [[WORD]] 2, label %[[W_DEST:[0-9]+]]
@@ -523,7 +526,10 @@ end:
 sil @single_payload_no_xi_switch_arg : $@convention(thin) (SinglePayloadNoXI2) -> () {
 // CHECK: entry:
 entry(%u : $SinglePayloadNoXI2):
-// CHECK:   br i1 %1, label %[[TAGS:[0-9]+]], label %[[X_PREDEST:[0-9]+]]
+// CHECK:   switch i1 %1, label %[[DFLT:[0-9]+]] [
+// CHECK:     i1 false, label %[[X_PREDEST:[0-9]+]]
+// CHECK:     i1 true, label %[[TAGS:[0-9]+]]
+// CHECK:   ]
 // CHECK: ; <label>:[[TAGS]]
 // CHECK:   switch [[WORD]] %0, label %[[DFLT:[0-9]+]] [
 // CHECK:     [[WORD]] 0, label %[[Y_DEST:[0-9]+]]