Page MenuHomePhabricator

[IR][LoopRotate] avoid leaving phi with no operands (PR48296)
ClosedPublic

Authored by spatel on Nov 27 2020, 1:12 PM.

Details

Summary

https://llvm.org/PR48296 shows an example where we delete all of the operands of a phi without actually deleting the phi, and that is invalid IR. The reduced test included here currently crashes for that reason. This was updated relatively recently in D80141.

I'm not familiar with all of the callers and use cases, but it seems there are passes that expect to handle deleting such a phi at a later time. We get regression test failures if we just stub out the DeletePHIIfEmpty argument in removeIncomingValue() for example.

So I changed the logic in BasicBlock::removePredecessor() to allow deletion of the phi if it has only one predecessor to start with because removing the lone predecessor is guaranteed to produce invalid IR in that case. This does not appear to cause problems in other passes.

Diff Detail

Event Timeline

spatel created this revision.Nov 27 2020, 1:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 27 2020, 1:12 PM
spatel requested review of this revision.Nov 27 2020, 1:12 PM

I think the bug is downstream - removePredecessor()'s default for KeepOneInputPHIs is false,
so someone explicitly asked for this behaviour.

I think the bug is downstream - removePredecessor()'s default for KeepOneInputPHIs is false,
so someone explicitly asked for this behaviour.

For this example, we're getting to removePredecessor() from llvm::changeToUnreachable(), where PreserveLCSSA is set and becomes KeepOneInputPHIs. If I change that to ignore PreserveLCSSA, then we get a different crash. So there's apparently some valid reason to keep the behavior where the phi ends up with 1 predecessor vs. ends up with 0 predecessors.

lebedev.ri added inline comments.Nov 27 2020, 2:30 PM
llvm/lib/IR/BasicBlock.cpp
320–321

There's also a comment at the declaration of the function

321–346

Ok, but i think this is rather akward.
It should be more like this.
Note that removeIncomingValue() *only* removes PHI's if they had a single pred,
so it doesn't make sense to pass anything related to KeepOneInputPHIs to it.

I don't think this is the right way to go, this breaks the invariant that # inputs to a phi node should match the # predecessors to a block. I'd recommend allowing zero input phi nodes in zero-predecessor blocks. I'm not sure why we ban zero input phi nodes right now.

spatel updated this revision to Diff 308213.Nov 29 2020, 7:40 AM

Patch updated:
Pre-committed the suggested cleanups ( 2cebad7 , ce134da ), so this patch becomes even simpler.

I don't think this is the right way to go, this breaks the invariant that # inputs to a phi node should match the # predecessors to a block. I'd recommend allowing zero input phi nodes in zero-predecessor blocks. I'm not sure why we ban zero input phi nodes right now.

I'm not seeing how this would break the assumption (and maybe that's clearer with the updated patch?), but that's an interesting question and independent of this fix IIUC.

The verifier assertion says:

// Ensure that PHI nodes have at least one entry!
Assert(PN.getNumIncomingValues() != 0,
       "PHI nodes must have at least one entry.  If the block is dead, "
       "the PHI should be removed!",
       &PN);

But if we're in an unreachable block, then we allow other IR forms that are even less expected (self-reference), so we should loosen this restriction?

lebedev.ri accepted this revision.Nov 29 2020, 8:06 AM

LGTM, thanks.

This only matters for unreachable blocks,
so i think i agree that the assertion should be relaxed,
but this is a reasonable cleanup regardless.

This revision is now accepted and ready to land.Nov 29 2020, 8:06 AM

But if we're in an unreachable block, then we allow other IR forms that are even less expected (self-reference), so we should loosen this restriction?

Yes, I'm suggesting we delete that assertion entirely and just check that # entries = # predecessors.

foad added inline comments.Nov 30 2020, 2:26 AM
llvm/lib/IR/BasicBlock.cpp
333

FYI I tried this as part of D80206 but had to revert because of https://bugs.llvm.org/show_bug.cgi?id=46343.

spatel added inline comments.Nov 30 2020, 4:35 AM
llvm/lib/IR/BasicBlock.cpp
333

Hmm...I tried the test case from that report with this patch, and I don't see crashing. D80206 was bigger than this change, so something else might be needed to trigger the bug. It's also possible that jump-threading got better about ignoring dead code?

If there are no objections, I think we should make this change just to make things simpler. I can look at relaxing the assert as a next step.

foad added inline comments.Nov 30 2020, 4:44 AM
llvm/lib/IR/BasicBlock.cpp
333

No objections from me.