From c60b51be690637a43e577b2abae17f4f75cc1791 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Tue, 26 May 2015 06:06:43 +0000 Subject: [PATCH] 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 | 25 ++---- lib/IRGen/EnumPayload.h | 11 +-- lib/IRGen/GenEnum.cpp | 160 +++++++++++++++----------------------- test/IRGen/enum.sil | 12 ++- 4 files changed, 79 insertions(+), 129 deletions(-) diff --git a/lib/IRGen/EnumPayload.cpp b/lib/IRGen/EnumPayload.cpp index 15fcc045c4e..7a3016584d0 100644 --- a/lib/IRGen/EnumPayload.cpp +++ b/lib/IRGen/EnumPayload.cpp @@ -357,7 +357,7 @@ static void emitSubSwitch(IRGenFunction &IGF, MutableArrayRef values, APInt mask, MutableArrayRef> 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> 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; } diff --git a/lib/IRGen/EnumPayload.h b/lib/IRGen/EnumPayload.h index 227e73a4dac..0822cad9244 100644 --- a/lib/IRGen/EnumPayload.h +++ b/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; - /// 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> cases, - SwitchDefaultDest dflt) const; + llvm::BasicBlock *dflt) const; /// Emit an equality comparison operation that payload & mask == value. llvm::Value *emitCompare(IRGenFunction &IGF, diff --git a/lib/IRGen/GenEnum.cpp b/lib/IRGen/GenEnum.cpp index 89678087fe3..09de27a3a56 100644 --- a/lib/IRGen/GenEnum.cpp +++ b/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 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()) @@ -1407,73 +1438,12 @@ namespace { ++elti; return result; }; - - // If there are extra tag bits, switch over them first. - SmallVector 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, 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, - SwitchDefaultDest(payloadDest, IsNotUnreachable)); + payload.emitSwitch(IGF, mask, cases, payloadDest); } // 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"); - // Handle the cases covered by each tag bit value. - // If there was only one no-payload case, or the payload is empty, we - // already branched in the first switch. - if (PayloadBitCount > 0 && ElementsWithNoPayload.size() > 1) { - unsigned casesPerTag = PayloadBitCount >= 32 - ? UINT_MAX : 1U << PayloadBitCount; + // 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. + unsigned casesPerTag = payloadBits >= 32 ? UINT_MAX : 1U << payloadBits; for (unsigned i = 1, e = tagBitBlocks.size(); i < e; ++i) { assert(elti != eltEnd && "ran out of cases before running out of extra tags?"); @@ -1508,19 +1484,15 @@ namespace { SmallVector, 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 - // whose extra inhabitants differ. - payload.emitSwitch(IGF, APInt::getAllOnesValue(PayloadBitCount), - cases, - SwitchDefaultDest(unreachableBB, IsUnreachable)); + // FIXME: Provide a mask for + payload.emitSwitch(IGF, APInt::getAllOnesValue(payloadBits), cases, + unreachableBB); } } - - 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. @@ -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, - extraBits, zero); - } + llvm::Value *zero = llvm::ConstantInt::get(extraBits->getType(), 0); + llvm::Value *isZero = IGF.Builder.CreateICmp(llvm::CmpInst::ICMP_EQ, + extraBits, zero); - auto *zeroBB = llvm::BasicBlock::Create(IGF.IGM.getLLVMContext()); - IGF.Builder.CreateCondBr(isNonzero, nonzeroBB, zeroBB); + auto *trueBB = llvm::BasicBlock::Create(IGF.IGM.getLLVMContext()); + 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, - SwitchDefaultDest(payloadBB, IsNotUnreachable)); + payload.emitSwitch(IGF, mask, cases, payloadBB); 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, - SwitchDefaultDest(unreachableBB, IsUnreachable)); + cases, unreachableBB); ++tagIndex; } diff --git a/test/IRGen/enum.sil b/test/IRGen/enum.sil index b3e7326e153..f6b3608b14f 100644 --- a/test/IRGen/enum.sil +++ b/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: ;