Page MenuHomePhabricator

[IPSCCP] Support unfeasible default dests for switch.
AcceptedPublic

Authored by fhahn on Tue, Nov 9, 10:40 AM.

Details

Summary

At the moment, unfeasible default destinations are not handled properly
in removeNonFeasibleEdges. So far, only unfeasible cases are removed,
but later code expects unreachable blocks to have no predecessors.

This is causing the crash reported in PR49573.

If the default destination is unfeasible it won't be executed. Create
a new unreachable block on demand and use that as default
destination.

Note that at the moment this only is relevant for cases where
resolvedUndefsIn marks the first case as executable. Regular switch
handling has a FIXME/TODO to support determining whether the default
case is feasible or not.

Diff Detail

Event Timeline

fhahn created this revision.Tue, Nov 9, 10:40 AM
fhahn requested review of this revision.Tue, Nov 9, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 9, 10:40 AM

Not really familiar with this pass, but the transform sounds legal, although i would have guessed you'd want to instead point it to an new block with unreachable?

nikic added a comment.Thu, Nov 25, 9:20 AM

Agree with @lebedev.ri, this should point to an unreachable block. It doesn't matter much for the resolvedUndefsIn case, but with proper support merging into an unrelated case would be a pretty bad choice.

fhahn updated this revision to Diff 390028.Fri, Nov 26, 5:18 AM

Not really familiar with this pass, but the transform sounds legal, although i would have guessed you'd want to instead point it to an new block with unreachable?

! In D113497#3154265, @nikic wrote:

Agree with @lebedev.ri, this should point to an unreachable block. It doesn't matter much for the resolvedUndefsIn case, but with proper support merging into an unrelated case would be a pretty bad choice.

Thanks, I updated the code to create a new unreachable block on demand (and cache it for the function) and use that as default successor.

fhahn edited the summary of this revision. (Show Details)Fri, Nov 26, 5:19 AM
lebedev.ri accepted this revision.Fri, Nov 26, 5:32 AM

LG.

If SCCP runs again, will it then again create a new unreachable block
for the default that is already pointing at the unreachable block?
Should it not do such redundant work?

This revision is now accepted and ready to land.Fri, Nov 26, 5:32 AM
fhahn added a comment.Mon, Nov 29, 3:09 AM

LG.

If SCCP runs again, will it then again create a new unreachable block
for the default that is already pointing at the unreachable block?
Should it not do such redundant work?

Good point. In practice I think IPSCCP is only run twice when doing LTO, so it's probably not a big deal. But let me check if the default dst is already unreachable.