[PATCH] Expand SimplifyCFG to convert certain simple switches to selects

[Philip Reames]

I really like the overall direction of this patch, but it could use a bit of cleanup for readability.  

You might also be able to generalize some of the transform:
- single unique value + default
- default probably dead without transform

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3686
@@ +3685,3 @@
+    Constant *&DefaultResult) {
+  for (auto I = SI->case_begin(), E = SI->case_end(); I != E; ++I) {
+    ConstantInt *CaseVal = I.getCaseValue();
----------------
range loop?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3700
@@ +3699,3 @@
+    // intermediate unconditional block to the common destination.
+    if (CaseSuccessor != CommonDest &&
+        !(BI && BI->isUnconditional() && BI->getSuccessor(0) == CommonDest))
----------------
split the two early exit cases?  might improve readability, particularly if the CaseSuccessor != CommonDest check is above BI.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3713
@@ +3712,3 @@
+      return false;
+    if (ResultFound == UniqueResults.end()) {
+      UniqueResults[Result.second].push_back(CaseVal);
----------------
You might restructure this to put ResultFound closer to it's use and separate the two if/else clauses.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3725
@@ +3724,3 @@
+                 DL);
+  // No default case makes us assume that default is not possible.
+  DefaultResult =
----------------
It looks like you might end up with PHI unset if you had a switch with only a default case.  Could such a switch reach here?  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3766
@@ +3765,3 @@
+// select.
+static Value *
+ConvertTwoCaseSwitch(const SmallVectorImpl<ConstantInt *> &FirstGroupCases,
----------------
It looks like this code is actually handling two cases + default.  Update the comment?

Also, is it always profitable to replace one switch with two selects?  I would guess not, but will defer to actual measurement.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3814
@@ +3813,3 @@
+// transform a switch with cases arranged into two ordered not overlapping
+// groups into a value select
+static Value *
----------------
Could you add examples to your function comments?  It would make them a bit more obvious.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3902
@@ +3901,3 @@
+  // Selects choose between two values only.
+  if (UniqueResults.size() != 2)
+    return false;
----------------
Since you're handling defaults, wouldn't it also make sense to handle one unique result and a default?  i.e.
switch(i) {
case 4,5,6: return 5;
default: return 0;
}

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3955
@@ +3954,3 @@
+                          (FirstPossibleHits + SecondPossibleHits)) != 0);
+    DefaultCanTrigger &= DefaultResult != nullptr;
+
----------------
It seems like proving the switch default is dead is a useful optimization even if we can make no other changes.  Should this be separated and generalized?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3964
@@ +3963,3 @@
+  // try finding a bit pattern that covers the cases and test on that
+  if (SelectValue == nullptr) {
+    SelectValue = ConvertBitPatternSwitch(
----------------
The general convention used is an early return when a change is made, not a signal value (nullptr here) when a change isn't made.  In this particular example, I believe using that pattern would make the code much easier to follow.  

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3974
@@ +3973,3 @@
+
+  PHI->addIncoming(SelectValue, SI->getParent());
+
----------------
Everything from here on could easily be a helper function.

http://reviews.llvm.org/D4219