Page MenuHomePhabricator

Jump threading: avoid trying to split edge into landingpad block (PR27840)
ClosedPublic

Authored by hans on Sep 16 2016, 12:57 PM.

Diff Detail

Event Timeline

hans updated this revision to Diff 71691.Sep 16 2016, 12:57 PM
hans retitled this revision from to Jump threading: avoid trying to split edge into landingpad block (PR27840).
hans updated this object.
hans added a reviewer: nlewycky.
hans added a subscriber: llvm-commits.
nlewycky edited edge metadata.Sep 16 2016, 4:22 PM

I'm concerned that the fix is too narrow. There's 10 callers to llvm::SplitEdge in LLVM and haven't audited them so I don't know whether they also check whether they also do this check. It's certainly not in the comment on SplitEdge that it's allowed to assert if you pass it a landing pad block.

hans added a comment.Sep 21 2016, 10:11 AM

I'm concerned that the fix is too narrow. There's 10 callers to llvm::SplitEdge in LLVM and haven't audited them so I don't know whether they also check whether they also do this check. It's certainly not in the comment on SplitEdge that it's allowed to assert if you pass it a landing pad block.

My thinking was that it seems what jump threading is trying to do with a "branch on xor" pattern isn't going to work with a landingpad anyway, and this would fix the crash that people run into in practice.

I agree it doesn't fix the wider problem of what to do when SplitEdge fails, but it seems like a good improvement to me.

Ping?

Also +Kyle, who I think has looked at jump threading recently.

While I agree there's a bigger problem with SplitEdge failing, I'd like to commit this fix as the transformation this code is trying to do clearly won't work for landingpads, and users are getting crashes on this in the wild.

iteratee accepted this revision.Oct 3 2016, 11:11 AM
iteratee edited edge metadata.

I didn't look very closely at jump threading, but nick said to land this patch via email on Sep 23, so I'm comfortable accepting it based on that.

This revision is now accepted and ready to land.Oct 3 2016, 11:11 AM
hans added a comment.Oct 3 2016, 11:25 AM

Sorry for the mess here.

I missed Nick's emailed replies because they didn't show up on Phabricator, and while they did go to the mailing list, I wasn't on the to: or cc: line so my filter didn't catch it.

I'll land this and keep the PR open as suggested by Nick.

hans closed this revision.Oct 3 2016, 11:32 AM

r283129