[cfe-commits] r54006 - in /cfe/trunk: lib/CodeGen/CGStmt.cpp test/CodeGen/rdr-6098585-default-after-caserange.c test/CodeGen/rdr-6098585-default-fallthrough-to-caserange.c test/CodeGen/rdr-6098585-empty-case-range.c test/CodeGen/rdr-6098585-fallthrough-to-empty-range.c test/CodeGen/rdr-6098585-unsigned-caserange.c
Daniel Dunbar
daniel at zuster.org
Thu Jul 24 18:11:38 PDT 2008
Author: ddunbar
Date: Thu Jul 24 20:11:38 2008
New Revision: 54006
URL: http://llvm.org/viewvc/llvm-project?rev=54006&view=rev
Log:
Rework codegen of case ranges
- Fix multiple issues with the way case ranges were emitted, see test
cases details about the specific issues.
- The root issue was not being careful about how basic blocks were
emitted which led to them being chained together incorrectly,
resulting in improper control flow.
- Fixes <rdar://problem/6098585>
Added:
cfe/trunk/test/CodeGen/rdr-6098585-default-after-caserange.c
cfe/trunk/test/CodeGen/rdr-6098585-default-fallthrough-to-caserange.c
cfe/trunk/test/CodeGen/rdr-6098585-empty-case-range.c
cfe/trunk/test/CodeGen/rdr-6098585-fallthrough-to-empty-range.c
cfe/trunk/test/CodeGen/rdr-6098585-unsigned-caserange.c
Modified:
cfe/trunk/lib/CodeGen/CGStmt.cpp
Modified: cfe/trunk/lib/CodeGen/CGStmt.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGStmt.cpp?rev=54006&r1=54005&r2=54006&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGStmt.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGStmt.cpp Thu Jul 24 20:11:38 2008
@@ -120,10 +120,10 @@
if (LastBB->getTerminator()) {
// If the previous block is already terminated, don't touch it.
- } else if (LastBB->empty() && LastBB->getValueName() == 0) {
+ } else if (LastBB->empty() && isDummyBlock(LastBB)) {
// If the last block was an empty placeholder, remove it now.
// TODO: cache and reuse these.
- Builder.GetInsertBlock()->eraseFromParent();
+ LastBB->eraseFromParent();
} else {
// Otherwise, create a fall-through branch.
Builder.CreateBr(BB);
@@ -420,44 +420,46 @@
/// add multiple cases to switch instruction, one for each value within
/// the range. If range is too big then emit "if" condition check.
void CodeGenFunction::EmitCaseStmtRange(const CaseStmt &S) {
+ // XXX kill me with param - ddunbar
assert(S.getRHS() && "Expected RHS value in CaseStmt");
llvm::APSInt LHS = S.getLHS()->getIntegerConstantExprValue(getContext());
llvm::APSInt RHS = S.getRHS()->getIntegerConstantExprValue(getContext());
+ // Emit the code for this case. We do this first to make sure it is
+ // properly chained from our predecessor before generating the
+ // switch machinery to enter this block.
+ StartBlock("sw.bb");
+ llvm::BasicBlock *CaseDest = Builder.GetInsertBlock();
+ EmitStmt(S.getSubStmt());
+
// If range is empty, do nothing.
if (LHS.isSigned() ? RHS.slt(LHS) : RHS.ult(LHS))
return;
llvm::APInt Range = RHS - LHS;
- // FIXME: parameters such as this should not be hardcoded
+ // FIXME: parameters such as this should not be hardcoded.
if (Range.ult(llvm::APInt(Range.getBitWidth(), 64))) {
// Range is small enough to add multiple switch instruction cases.
- StartBlock("sw.bb");
- llvm::BasicBlock *CaseDest = Builder.GetInsertBlock();
for (unsigned i = 0, e = Range.getZExtValue() + 1; i != e; ++i) {
SwitchInsn->addCase(llvm::ConstantInt::get(LHS), CaseDest);
LHS++;
}
- EmitStmt(S.getSubStmt());
return;
}
- // The range is too big. Emit "if" condition.
- llvm::BasicBlock *FalseDest = NULL;
- llvm::BasicBlock *CaseDest = llvm::BasicBlock::Create("sw.bb");
-
- // If we have already seen one case statement range for this switch
- // instruction then piggy-back otherwise use default block as false
- // destination.
- if (CaseRangeBlock)
- FalseDest = CaseRangeBlock;
- else
- FalseDest = SwitchInsn->getDefaultDest();
-
- // Start new block to hold case statement range check instructions.
- StartBlock("case.range");
- CaseRangeBlock = Builder.GetInsertBlock();
+ // The range is too big. Emit "if" condition into a new block,
+ // making sure to save and restore the current insertion point.
+ llvm::BasicBlock *RestoreBB = Builder.GetInsertBlock();
+
+ // Push this test onto the chain of range checks (which terminates
+ // in the default basic block). The switch's default will be changed
+ // to the top of this chain after switch emission is complete.
+ llvm::BasicBlock *FalseDest = CaseRangeBlock;
+ CaseRangeBlock = llvm::BasicBlock::Create("sw.caserange");
+
+ CurFn->getBasicBlockList().push_back(CaseRangeBlock);
+ Builder.SetInsertPoint(CaseRangeBlock);
// Emit range check.
llvm::Value *Diff =
@@ -467,9 +469,8 @@
Builder.CreateICmpULE(Diff, llvm::ConstantInt::get(Range), "tmp");
Builder.CreateCondBr(Cond, CaseDest, FalseDest);
- // Now emit case statement body.
- EmitBlock(CaseDest);
- EmitStmt(S.getSubStmt());
+ // Restore the appropriate insertion point.
+ Builder.SetInsertPoint(RestoreBB);
}
void CodeGenFunction::EmitCaseStmt(const CaseStmt &S) {
@@ -487,9 +488,9 @@
}
void CodeGenFunction::EmitDefaultStmt(const DefaultStmt &S) {
- StartBlock("sw.default");
- // Current insert block is the default destination.
- SwitchInsn->setSuccessor(0, Builder.GetInsertBlock());
+ llvm::BasicBlock *DefaultBlock = SwitchInsn->getDefaultDest();
+ assert(DefaultBlock->empty() && "EmitDefaultStmt: Default block already defined?");
+ EmitBlock(DefaultBlock);
EmitStmt(S.getSubStmt());
}
@@ -499,15 +500,18 @@
// Handle nested switch statements.
llvm::SwitchInst *SavedSwitchInsn = SwitchInsn;
llvm::BasicBlock *SavedCRBlock = CaseRangeBlock;
- CaseRangeBlock = NULL;
- // Create basic block to hold stuff that comes after switch statement.
- // Initially use it to hold DefaultStmt.
- llvm::BasicBlock *NextBlock = llvm::BasicBlock::Create("after.sw");
- SwitchInsn = Builder.CreateSwitch(CondV, NextBlock);
+ // Create basic block to hold stuff that comes after switch
+ // statement. We also need to create a default block now so that
+ // explicit case ranges tests can have a place to jump to on
+ // failure.
+ llvm::BasicBlock *NextBlock = llvm::BasicBlock::Create("sw.epilog");
+ llvm::BasicBlock *DefaultBlock = llvm::BasicBlock::Create("sw.default");
+ SwitchInsn = Builder.CreateSwitch(CondV, DefaultBlock);
+ CaseRangeBlock = DefaultBlock;
// Create basic block for body of switch
- StartBlock("body.sw");
+ StartBlock("sw.body");
// All break statements jump to NextBlock. If BreakContinueStack is non empty
// then reuse last ContinueBlock.
@@ -520,22 +524,20 @@
EmitStmt(S.getBody());
BreakContinueStack.pop_back();
- // If one or more case statement range is seen then use CaseRangeBlock
- // as the default block. False edge of CaseRangeBlock will lead to
- // original default block.
- if (CaseRangeBlock)
- SwitchInsn->setSuccessor(0, CaseRangeBlock);
-
- // Prune insert block if it is dummy.
- llvm::BasicBlock *BB = Builder.GetInsertBlock();
- if (isDummyBlock(BB))
- BB->eraseFromParent();
- else // Otherwise, branch to continuation.
- Builder.CreateBr(NextBlock);
+ // Update the default block in case explicit case range tests have
+ // been chained on top.
+ SwitchInsn->setSuccessor(0, CaseRangeBlock);
+
+ // If a default was never emitted then reroute any jumps to it and
+ // discard.
+ if (!DefaultBlock->getParent()) {
+ DefaultBlock->replaceAllUsesWith(NextBlock);
+ delete DefaultBlock;
+ }
+
+ // Emit continuation.
+ EmitBlock(NextBlock);
- // Place NextBlock as the new insert point.
- CurFn->getBasicBlockList().push_back(NextBlock);
- Builder.SetInsertPoint(NextBlock);
SwitchInsn = SavedSwitchInsn;
CaseRangeBlock = SavedCRBlock;
}
Added: cfe/trunk/test/CodeGen/rdr-6098585-default-after-caserange.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/rdr-6098585-default-after-caserange.c?rev=54006&view=auto
==============================================================================
--- cfe/trunk/test/CodeGen/rdr-6098585-default-after-caserange.c (added)
+++ cfe/trunk/test/CodeGen/rdr-6098585-default-after-caserange.c Thu Jul 24 20:11:38 2008
@@ -0,0 +1,18 @@
+// RUN: clang --emit-llvm-bc -o - %s | opt -std-compile-opts | llvm-dis > %t &&
+// RUN: grep "ret i32" %t | count 1 &&
+// RUN: grep "ret i32 10" %t | count 1
+
+// Ensure that default after a case range is not ignored.
+
+static int f1(unsigned x) {
+ switch(x) {
+ case 10 ... 0xFFFFFFFF:
+ return 0;
+ default:
+ return 10;
+ }
+}
+
+int g() {
+ return f1(2);
+}
Added: cfe/trunk/test/CodeGen/rdr-6098585-default-fallthrough-to-caserange.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/rdr-6098585-default-fallthrough-to-caserange.c?rev=54006&view=auto
==============================================================================
--- cfe/trunk/test/CodeGen/rdr-6098585-default-fallthrough-to-caserange.c (added)
+++ cfe/trunk/test/CodeGen/rdr-6098585-default-fallthrough-to-caserange.c Thu Jul 24 20:11:38 2008
@@ -0,0 +1,20 @@
+// RUN: clang --emit-llvm-bc -o - %s | opt -std-compile-opts | llvm-dis > %t &&
+// RUN: grep "ret i32 10" %t
+
+// Ensure that this doesn't compile to infinite loop in g() due to
+// miscompilation of fallthrough from default to a (tested) case
+// range.
+
+static int f0(unsigned x) {
+ switch(x) {
+ default:
+ x += 1;
+ case 10 ... 0xFFFFFFFF:
+ return 0;
+ }
+}
+
+int g() {
+ f0(1);
+ return 10;
+}
Added: cfe/trunk/test/CodeGen/rdr-6098585-empty-case-range.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/rdr-6098585-empty-case-range.c?rev=54006&view=auto
==============================================================================
--- cfe/trunk/test/CodeGen/rdr-6098585-empty-case-range.c (added)
+++ cfe/trunk/test/CodeGen/rdr-6098585-empty-case-range.c Thu Jul 24 20:11:38 2008
@@ -0,0 +1,23 @@
+// RUN: clang --emit-llvm-bc -o - %s | opt -std-compile-opts | llvm-dis > %t &&
+// RUN: grep "ret i32" %t | count 2 &&
+// RUN: grep "ret i32 3" %t | count 2
+
+// This generated incorrect code because of poor switch chaining.
+int f1(int x) {
+ switch(x) {
+ default:
+ return 3;
+ case 10 ... 0xFFFFFFFF:
+ return 0;
+ }
+}
+
+// This just asserted because of the way case ranges were calculated.
+int f2(int x) {
+ switch (x) {
+ default:
+ return 3;
+ case 10 ... -1:
+ return 0;
+ }
+}
Added: cfe/trunk/test/CodeGen/rdr-6098585-fallthrough-to-empty-range.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/rdr-6098585-fallthrough-to-empty-range.c?rev=54006&view=auto
==============================================================================
--- cfe/trunk/test/CodeGen/rdr-6098585-fallthrough-to-empty-range.c (added)
+++ cfe/trunk/test/CodeGen/rdr-6098585-fallthrough-to-empty-range.c Thu Jul 24 20:11:38 2008
@@ -0,0 +1,15 @@
+// RUN: clang --emit-llvm-bc -o - %s | opt -std-compile-opts | llvm-dis > %t &&
+// RUN: grep "ret i32 %" %t
+
+// Make sure return is not constant (if empty range is skipped or miscompiled)
+
+int f0(unsigned x) {
+ switch(x) {
+ case 2:
+ // fallthrough empty range
+ case 10 ... 9:
+ return 10;
+ default:
+ return 0;
+ }
+}
Added: cfe/trunk/test/CodeGen/rdr-6098585-unsigned-caserange.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/rdr-6098585-unsigned-caserange.c?rev=54006&view=auto
==============================================================================
--- cfe/trunk/test/CodeGen/rdr-6098585-unsigned-caserange.c (added)
+++ cfe/trunk/test/CodeGen/rdr-6098585-unsigned-caserange.c Thu Jul 24 20:11:38 2008
@@ -0,0 +1,12 @@
+// RUN: clang --emit-llvm-bc -o - %s | opt -std-compile-opts | llvm-dis > %t &&
+// RUN: grep "ret i32" %t | count 1 &&
+// RUN: grep "ret i32 3" %t | count 1
+
+int f2(unsigned x) {
+ switch(x) {
+ default:
+ return 3;
+ case 0xFFFFFFFF ... 1: // This range should be empty because x is unsigned.
+ return 0;
+ }
+}
More information about the cfe-commits
mailing list