[PATCH] Switch to Select optimisations

[Marcello Maggioni]

================
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 &&
----------------
hans wrote:
> Why is this check needed? Doesn't GetCaseResults already check that the case leads to the common destination unconditionally?
Yeah, you are right. Thanks for this , makes it much cleaner!

================
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) {
----------------
hans wrote:
> 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?
The main problem of DenseMap is that it is unordered. We need to enforce the same order every run if we want repeatability of output. Using a vector here makes it easier and probably faster, because the typical size of the vector here is in that spot where actually linear scanning a vector is usually faster.

================
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)
----------------
hans wrote:
> I guess the appending happened above, so maybe just "find the PHI"?
Done

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

================
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 =
----------------
hans wrote:
> This doesn't seem to take into account that there could be other instructions in the default destination before the unreachable terminator.
Good spot! :) I added a check to also be sure the basic block contains only one instruction.

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

================
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
----------------
hans wrote:
> i assume the icmp should be with 10 here
Done :)

================
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) {
----------------
hans wrote:
> i would add an assert about ResultVector.size()
Done.

================
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
----------------
hans wrote:
> 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.
I have no preference on the matter, so I guess its ok :)

================
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
----------------
hans wrote:
> 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?
Mmm, this is a rather important optimization for the use case I'm trying to cover. Do you think it is really necessary to put it in a separate patch later on?

================
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
----------------
hans wrote:
> I don't think the "or more" case is supported, though?
Yeah, we support just one PHI node here :)

http://reviews.llvm.org/D5525