Page MenuHomePhabricator

[InstCombine] Don't combine PHI before catchswitch
ClosedPublic

Authored by aheejin on Jul 1 2021, 12:30 PM.

Details

Summary

This tries to bail out if the PHI is in a catchswitch BB in
InstCombine. A PHI cannot be combined into a non-PHI instruction if it
is in a catchswitch BB, because catchswitch BB cannot have any
non-PHI instruction other than catchswitch itself.

The given test case started crashing after D98058.

Diff Detail

Event Timeline

aheejin created this revision.Jul 1 2021, 12:30 PM
aheejin requested review of this revision.Jul 1 2021, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2021, 12:30 PM
lebedev.ri added inline comments.
llvm/test/Transforms/InstCombine/catchswitch-phi.ll
2

This isn't testing anything

This is seems like a too heavy hammer.
foldPHIArgZextsIntoPHI() has:

// We cannot create a new instruction after the PHI if the terminator is an
// EHPad because there is no valid insertion point.
if (Instruction *TI = Phi.getParent()->getTerminator())
  if (TI->isEHPad())
    return nullptr;

Is that the same check as here? (which means that the check here is incomplete?)
If yes, then can you add a similar check to whatever fold is missing it, specifically?

rnk added inline comments.Jul 1 2021, 1:14 PM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1329

Every other return path of InstCombinerImpl::visitPHINode appears to create and return an existing instruction of a new phi node. This one is special in that it creates a new non-phi instruction.

The insertion location is not determined here, but it is predictable, it's the first available insertion point after the phi, but the phi's block has no insertion point. The generic way to test for this is to compare getInsertionPoint against the end iterator.

So, all the other transforms seem fine. It's reasonable to put a more targeted fix right here:

&& PN.getParent()->getFirstNonPHI() != PN.getParent()->end()
llvm/test/Transforms/InstCombine/catchswitch-phi.ll
2

That's true, and I don't see any cast instructions to trigger the codepath we are looking at.

aheejin updated this revision to Diff 356114.Jul 2 2021, 12:33 AM
aheejin marked 3 inline comments as done.

Address comments

aheejin added a comment.EditedJul 2 2021, 12:33 AM

This is seems like a too heavy hammer.
foldPHIArgZextsIntoPHI() has:

// We cannot create a new instruction after the PHI if the terminator is an
// EHPad because there is no valid insertion point.
if (Instruction *TI = Phi.getParent()->getTerminator())
  if (TI->isEHPad())
    return nullptr;

Is that the same check as here? (which means that the check here is incomplete?)

This check is fine, but as we can see here, if the return value of foldPHIArgZextsIntoPHI is nullptr, it doesn't exit visitPHINode but goes on to the next optimization within the function, so it couldn't have prevented the crash.

If yes, then can you add a similar check to whatever fold is missing it, specifically?

I'll move the check to that specific part added in D98058.

llvm/test/Transforms/InstCombine/catchswitch-phi.ll
2

Oh sorry, I forgot to add the RUN line. Added now. Basically this shouldn't crash.
@rnk phi in bb4 is the one that triggers the crash (without this patch).

34

This is the instruction that triggers the crash without this patch

lebedev.ri accepted this revision.Jul 2 2021, 12:41 AM

LG with nits

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1319–1320
1320–1322

The fix should go here - what's the point of doing all that computation if we know by now that we are going to throw it away?

This revision is now accepted and ready to land.Jul 2 2021, 12:41 AM
aheejin marked 2 inline comments as done.Jul 2 2021, 1:08 AM
aheejin updated this revision to Diff 356121.Jul 2 2021, 1:08 AM

Address comments

Thank you for the feedback!

rnk accepted this revision.Jul 2 2021, 11:43 AM

lgtm

llvm/test/Transforms/InstCombine/catchswitch-phi.ll
2

I think I was looking for bitcast instructions, but the zero-index GEPs are essentially cast instructions here. Looks good.

aheejin edited the summary of this revision. (Show Details)Jul 2 2021, 12:10 PM
This revision was automatically updated to reflect the committed changes.