[PATCH] D29107: Fix a bug when unswitching on partial LIV for SwitchInst

[Xin Tong via Phabricator via llvm-commits]

trentxintong added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:1060
+    // We are only unswitching full LIV.
+    BranchesInfo.setUnswitched(SI, CondVal);
     ++NumSwitches;
----------------
sanjoy wrote:
> I understand why you need this, but IIUC the `CurLoopInstructions` table is unnecessary.
> 
> We can solve the problem it solves in either of two ways:
> 
>  - Remove cases from switches after unswitching them.  That is, with `switch (V) { case 0: ... }` the `V != 0` version of the loop does not have a `case 0:`.  This would prevent us from unswitching on that condition again by definition.
> 
>  - Do not unswitch on cases that lead to a block that solely contains `unreachable`.  This way we will avoid doing redundant work generally (simplifycfg will clean these trivial cases up), and also prevent us unswitching `switch` es that we just unswitched.
> 
> So while your change itself is okay, depending on how much time you have it would be nice to remove `CurLoopInstructions` altogether.
> 
* Remove cases from switches after unswitching them.  I think this is possible.

* Do not unswitch on cases that lead to a block that solely contains unreachable. - We can not split critical edges with some obscure cases correctly right now, say we have 2 edges from BB-A to BB-B, when we call SplitEdge, we do not know which one is split. To be more specific, the one returned by GetSuccessorNumber(BB, Succ) is split. In such case we can not mark a case with unreachable because we may not get a basic block to put that unreachable instruction. This can be fixed as well.

I am going to land this for now. And think about what i can do to make this better sometime next month or so.


https://reviews.llvm.org/D29107