llvm dev - May 2020 - 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.

On 5/15/20 5:57 AM, Jay Foad via llvm-dev wrote:
> 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`.

> Thanks,
> Jay.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Fri, May 15, 2020 at 7:47 PM Johannes Doerfert via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> On 5/15/20 5:57 AM, Jay Foad via llvm-dev wrote:
> > 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

>
>
> > Thanks,
> > Jay.
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

On Fri, 15 May 2020 at 18:46, Johannes Doerfert
<johannesdoerfert at gmail.com> wrote:
> On 5/15/20 5:57 AM, Jay Foad via llvm-dev wrote:
> > 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`.
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.