[PATCH] D77620: [SimpleLoopUnswitch] Do not delete DT edge when a duplicate exists.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked an inline comment as done.
asbirlea added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:830
+    // If the deleted edge was not duplicated in the switch, delete from the DT.
+    if (!llvm::is_contained(successors(ParentBB), SplitUnswitchedPair.first))
+      DTUpdates.push_back({DT.Delete, ParentBB, SplitUnswitchedPair.first});
----------------
chandlerc wrote:
> (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:
> 
> 1) Narrow the special case to blocks containing *just* `unreachable`.
> 2) Apply in consistently to cases as well as the default destination.
> 
> 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.
Thanks a lot for the summary, this is great to have in writing!

I sent out D78277 and D78279 last week as a replacement for this patch. My apologies for forgetting to update the discussion here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77620/new/

https://reviews.llvm.org/D77620