[PATCH] Switch to Select optimisations

[Hans Wennborg]

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3457
@@ +3456,3 @@
+    // Support only the case where the switch branches directly or through an
+    // intermediate unconditional block to the common destination.
+    if (CaseSuccessor != CommonDest &&
----------------
Why is this check needed? Doesn't GetCaseResults already check that the case leads to the common destination unconditionally?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3466
@@ +3465,3 @@
+    auto ResultFound = 
+      std::find_if(UniqueResults.begin(), UniqueResults.end(),
+                   [&Result](const ResultVectorTy::value_type &t) {
----------------
If I understand correctly, UniqueResults is a mapping from result to vector of cases. Could we use a DenseMap instead and save some code here?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3478
@@ +3477,3 @@
+
+    // Append the result from this case to the list and find the PHI
+    if (PHI == nullptr)
----------------
I guess the appending happened above, so maybe just "find the PHI"?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3484
@@ +3483,3 @@
+  }
+  // Find the default result value
+  SmallVector<std::pair<PHINode *, Constant *>, 1> DefaultResults;
----------------
nit: a period at the end of comments is preferred.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3490
@@ +3489,3 @@
+  // If the default value is not found abort unless the default destination
+  // is unreachable.
+  DefaultResult =
----------------
This doesn't seem to take into account that there could be other instructions in the default destination before the unreachable terminator.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3497
@@ +3496,3 @@
+      (DefaultDest != CommonDest &&
+       !(BI && BI->isUnconditional() && BI->getSuccessor(0) == CommonDest)))
+    return false;
----------------
again, doesn't GetCaseResults already check that the case leads to CommonDest?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3537
@@ +3536,3 @@
+// switch (a) {
+//   case 10:                %0 = icmp eq i32 %a, 1
+//     return 10;            %1 = select i1 %0, i32 10, i32 4
----------------
i assume the icmp should be with 10 here

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3550
@@ +3549,3 @@
+  // select or a two-way select if default is possible.
+  if (ResultVector[0].second.size() == 1 &&
+      ResultVector[1].second.size() == 1) {
----------------
i would add an assert about ResultVector.size()

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3599
@@ +3598,3 @@
+  const bool SecondIsBigger =
+      MaxCaseFirst->getValue().slt(MinCaseSecond->getValue());
+  // If the case statements ranges do not overlap use a comparison to
----------------
Could these comparisons be done with sgt instead? I'd find that easier to read for code that checks whether a is bigger than b.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3617
@@ +3616,3 @@
+
+// ConvertBitPatternSwitch - Helper function that checks if it is possible to
+// transform a switch with cases that are groupable by some bit pattern into
----------------
I haven't had time to look at this one yet.

Do you think the bit pattern variant could be broken out to a separate patch?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3698
@@ +3697,3 @@
+
+/// SwitchToSelect - If the switch is only used to initialize one or more
+/// phi nodes in a common successor block with only two different
----------------
I don't think the "or more" case is supported, though?

http://reviews.llvm.org/D5525