SimplifyCFG doesn't proerly clean up trivially dead instructions

[efriedma-quic]

Bugzilla Link	2340
Resolution	FIXED
Resolved on	Dec 16, 2008 14:56
Version	unspecified
OS	Linux
Attachments	Testcase, Potential patch
CC	@nlewycky

Extended Description

To reproduce: run "opt -simplifycfg" on the attached testcase. Code after simplifycfg:

define i32 @​x(i32 %x) {
entry:
%call = call i32 (...)* @​foo( ) ; [#uses=0]
%cond = icmp eq i32 %x, 9 ; [#uses=0]
%call9 = call i32 (...)* @​bar( ) ; [#uses=0]
ret i32 0
}

The icmp is unused; SimplifyCFG should have eliminated it when it eliminated the branch that used it.

[lattner]

Hey Eli,

We have the same problem in many related passes. I think the best way to solve this is to grab "LoopStrengthReduce::DeleteTriviallyDeadInstructions", moving it to be a global function in Transform Utils [it would live with isInstructionTriviallyDead]. It would get a helper that takes a Value*, checks to see if it is an instruction, and if so, defines the SmallPtrSet on the stack, adds it, and then calls the recursive thing.

Given this, simplifycfg would just call this, passing in the condition Value*. Does this seem reasonable?

-Chris

[efriedma-quic]

Hey Eli,

We have the same problem in many related passes. I think the best way to solve
this is to grab "LoopStrengthReduce::DeleteTriviallyDeadInstructions", moving
it to be a global function in Transform Utils [it would live with
isInstructionTriviallyDead]. It would get a helper that takes a Value*, checks
to see if it is an instruction, and if so, defines the SmallPtrSet on the
stack, adds it, and then calls the recursive thing.

Given this, simplifycfg would just call this, passing in the condition Value*.
Does this seem reasonable?

Sure, but that seems orthogonal to the issue at hand; SimplifyCFG already has ErasePossiblyDeadInstructionTree, which does essentially the same thing.

[lattner]

Ah, good call. those should definitely be merged.

[lattner]

Your patch looks fine, except for cases where you have an unreachable/unwind instr. I'd prefer to keep I->eraseFromParent() for those, since they obviously have no condition.

[efriedma-quic]

Revised version committed.

[efriedma-quic]

Oops, forgot to mention this is r61100.