ConstantFoldTerminator doesn't delete trivially dead phi inputs

I'm looking into some missed optimizations where CodeGenPrepare seems
to leave trivially dead instructions lying around.

This happens because CodeGenPrepare::runOnFunction calls
ConstantFoldTerminator which folds a "br i1 false" into an
unconditional branch and calls BasicBlock::removePredecessor which
calls PHINode::removeIncomingValue. Each incoming value that is
removed from a phi node might be dead now, but nothing deletes them.

Is there any way this could be improved? Could
PHINode::removeIncomingValue call
RecursivelyDeleteTriviallyDeadInstructions on the value it removes, or
is that a layering violation? Any other ideas?

Thanks,
Jay.

I'm looking into some missed optimizations where CodeGenPrepare seems
to leave trivially dead instructions lying around.

This happens because CodeGenPrepare::runOnFunction calls
ConstantFoldTerminator which folds a "br i1 false" into an
unconditional branch and calls BasicBlock::removePredecessor which
calls PHINode::removeIncomingValue. Each incoming value that is
removed from a phi node might be dead now, but nothing deletes them.

Is there any way this could be improved? Could
PHINode::removeIncomingValue call
RecursivelyDeleteTriviallyDeadInstructions on the value it removes, or
is that a layering violation? Any other ideas?

I think this is certainly a desirable behavior that could be opt-in

for `ConstantFoldTerminator`.

> I'm looking into some missed optimizations where CodeGenPrepare seems
> to leave trivially dead instructions lying around.
>
> This happens because CodeGenPrepare::runOnFunction calls
> ConstantFoldTerminator which folds a "br i1 false" into an
> unconditional branch and calls BasicBlock::removePredecessor which
> calls PHINode::removeIncomingValue. Each incoming value that is
> removed from a phi node might be dead now, but nothing deletes them.
>
> Is there any way this could be improved? Could
> PHINode::removeIncomingValue call
> RecursivelyDeleteTriviallyDeadInstructions on the value it removes, or
> is that a layering violation?

I'd say it'd be surprising and could lead to crashes in users of
removeIncomingValue that happen to hold on to one of the now-dead
instructions for unrelated reasons.

I think this is certainly a desirable behavior that could be opt-in
for `ConstantFoldTerminator`.

This seems like a pretty reasonable way forward to me.

Cheers,
Nicolai

How's this for a possible implementation? https://reviews.llvm.org/D80206

I'm not sure if it needs to be opt-in, and/or whether it could
piggy-back on the existing DeleteDeadConditions argument.

Thanks,
Jay.