For the unswitched blocks that were split, there may be a duplicate edge
that was kept. Check this when updating the DominatorTree.
Note: Google Test filter = ConnectionFileDescriptorTest.TCPGetURIv6 [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
While this solution does solve the problem, there's an underlying bug that this is hiding. I've talking with Chandler about this and I'll send a couple of patches that are aiming to fix the underlying problem instead.
Apologies for the delay in getting you a fix.
(mostly summarizing our video chat discussion for the list and so I don't forget it)
The raw llvm::successors API is somewhat inefficient (switches over the instruction op-code).... And no matter what, this ends up being quadratic in the weird edge case where we unswitch half of the edges. For each unswitched edge that we split we'll walk every not-unswitched edge here.
But all of this I think is unnecessary. We shouldn't ever be leaving edges from the *switch instruction* to a basic block that we are unswitching. The split case is only intended to cover when there are edges to the basic block from other parts of the loop.
The way you get here is really fun: it uses a basic block ending in unreachable. Such blocks are special cased above in this routine, but only for the *default* destination. That's how we end up with there still being an edge: its the default edge to an unreachable-terminated block where we unswitch a case edge to that block.
So I think we need to make the handling of unreachable-terminated blocks uniform between defaults and cases, even though we don't *need* to special case them for cases (because we can delete cases entirely). Otherwise we will keep having surprises where they behave differently here.
But to do that, I think we need to *narrow* the special case significantly: currently it just checks that the block is terminated by unreachable. I think it should *also* ensure that is the only instruction in the block. For all such exiting edges of the loop, we don't need to both unswitching them. Notably, the case edges should be cleaned up by SimplifyCFG by deleting them, so there is no real need to do anything here IMO.
So I think that leaves two steps here:
I'm fine to do it in a single patch if it isn't too disruptive. Or we can do something vaguely similar to this patch as a temporary workaround that is easier to backport and then do #1 and #2 in to follow-up patches and remove the workaround.