This is an archive of the discontinued LLVM Phabricator instance.

LowerSwitch: Fix producing invalid IR on unreachable code
ClosedPublic

Authored by arsenm on Mar 13 2017, 3:28 PM.

Download Raw Diff

Details

Reviewers

Summary

If a switch was an unreachable block that branched to an unreachable block, it would leave phis with missing predecessors.

Diff Detail

Event Timeline

arsenm created this revision.Mar 13 2017, 3:28 PM

Herald added a subscriber: wdng. · View Herald TranscriptMar 13 2017, 3:28 PM

Handle branch to self also

ping

arsenm added a reviewer: hans.Apr 21 2017, 2:56 PM

lgtm

It's been a long time since I did anything with LowerSwitch, but this seems like a straight-forward fix.

lib/Transforms/Utils/LowerSwitch.cpp
406	I'd suggest a comment here.
test/Transforms/LowerSwitch/phi-in-dead-block.ll
4	Did we really get left with a switch before?

This revision is now accepted and ready to land.Apr 21 2017, 4:44 PM

arsenm added inline comments.Apr 21 2017, 4:53 PM

test/Transforms/LowerSwitch/phi-in-dead-block.ll
4	No. The verifier failed, but if I disable it the output has the expanded branches

r301064

Revision Contents

Path

Size

lib/

Transforms/

Utils/

LowerSwitch.cpp

6 lines

test/

Transforms/

LowerSwitch/

phi-in-dead-block.ll

40 lines

Diff 91639

lib/Transforms/Utils/LowerSwitch.cpp

	Show First 20 Lines • Show All 397 Lines • ▼ Show 20 Lines
	void LowerSwitch::processSwitchInst(SwitchInst *SI,			void LowerSwitch::processSwitchInst(SwitchInst *SI,
	SmallPtrSetImpl<BasicBlock*> &DeleteList) {			SmallPtrSetImpl<BasicBlock*> &DeleteList) {
	BasicBlock *CurBlock = SI->getParent();			BasicBlock *CurBlock = SI->getParent();
	BasicBlock *OrigBlock = CurBlock;			BasicBlock *OrigBlock = CurBlock;
	Function *F = CurBlock->getParent();			Function *F = CurBlock->getParent();
	Value *Val = SI->getCondition(); // The value we are switching on...			Value *Val = SI->getCondition(); // The value we are switching on...
	BasicBlock* Default = SI->getDefaultDest();			BasicBlock* Default = SI->getDefaultDest();

				if ((CurBlock != &F->getEntryBlock() && pred_empty(CurBlock)) \|\|
				hansUnsubmitted Not Done Reply Inline Actions I'd suggest a comment here. hans: I'd suggest a comment here.
				CurBlock->getSinglePredecessor() == CurBlock) {
				DeleteList.insert(CurBlock);
				return;
				}

	// If there is only the default destination, just branch.			// If there is only the default destination, just branch.
	if (!SI->getNumCases()) {			if (!SI->getNumCases()) {
	BranchInst::Create(Default, CurBlock);			BranchInst::Create(Default, CurBlock);
	SI->eraseFromParent();			SI->eraseFromParent();
	return;			return;
	}			}

	// Prepare cases vector.			// Prepare cases vector.
	▲ Show 20 Lines • Show All 110 Lines • Show Last 20 Lines

test/Transforms/LowerSwitch/phi-in-dead-block.ll

This file was added.

				; RUN: opt -S -lowerswitch %s \| FileCheck %s

				; CHECK-LABEL: @phi_in_dead_block(
				; CHECK-NOT: switch
				hansUnsubmitted Not Done Reply Inline Actions Did we really get left with a switch before? hans: Did we really get left with a switch before?
				arsenmAuthorUnsubmitted Not Done Reply Inline Actions No. The verifier failed, but if I disable it the output has the expanded branches arsenm: No. The verifier failed, but if I disable it the output has the expanded branches
				define void @phi_in_dead_block() {
				bb:
				br i1 undef, label %bb2, label %bb3

				bb1: ; No predecessors!
				switch i32 undef, label %bb2 [
				i32 9, label %bb3
				]

				bb2: ; preds = %bb1, %bb
				%tmp = phi i64 [ undef, %bb1 ], [ undef, %bb ]
				unreachable

				bb3: ; preds = %bb1, %bb
				unreachable
				}

				; CHECK-LABEL: @phi_in_dead_block_br_to_self(
				; CHECK-NOT: switch
				define void @phi_in_dead_block_br_to_self() {
				bb:
				br i1 undef, label %bb2, label %bb3

				bb1: ; No predecessors!
				switch i32 undef, label %bb2 [
				i32 9, label %bb3
				i32 10, label %bb1
				]

				bb2: ; preds = %bb1, %bb
				%tmp = phi i64 [ undef, %bb1 ], [ undef, %bb ]
				unreachable

				bb3: ; preds = %bb1, %bb
				unreachable
				}