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

[Marcello Maggioni]

================
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))
----------------
Philip Reames wrote:
> split the two early exit cases?  might improve readability, particularly if the CaseSuccessor != CommonDest check is above BI.  
The conditions for the exit cases here are &&ed and not ||ed . So the exit case here is actually one (when both the conditions are verified). What exactly do you mean here?

================
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 *
----------------
Philip Reames wrote:
> Could you add examples to your function comments?  It would make them a bit more obvious.  
Very good point, I'll do that definitely in the next revision

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3902
@@ +3901,3 @@
+  // Selects choose between two values only.
+  if (UniqueResults.size() != 2)
+    return false;
----------------
Philip Reames wrote:
> 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;
> }
This is a good idea. I need to check if that is profitable in performance though (needs two comparisons to check if the value falls in the range). If it seems profitable I'll add it.

================
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(
----------------
Philip Reames wrote:
> 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.  
I see, I will make the change, thanks.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3974
@@ +3973,3 @@
+
+  PHI->addIncoming(SelectValue, SI->getParent());
+
----------------
Philip Reames wrote:
> Everything from here on could easily be a helper function.
This is quite needed in order to make the change you requested in the comment above

http://reviews.llvm.org/D4219