[PATCH] Switch to Select optimisations

[Hans Wennborg]

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3556
@@ +3555,3 @@
+    Value *SelectValue = ResultVector[1].first;
+    if (DefaultCanTrigger) {
+      Value *const ValueCompare =
----------------
kariddi wrote:
> hans wrote:
> > Actually, I'd just do
> > 
> >   if (DefaultResult)
> > 
> > here.
> Hmm, I prefer here changing DefaultCanTrigger to :
> 
> DefaultCanTrigger = DefaultResult;
> 
> and then use DefaultCanTrigger in the if, because that is used everywhere else we need to check if the default can trigger, so seems a little bit more coherent in my opinion. (Makes more clear to the reader what we want to test for)
> What do you think?
That's fine.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3523
@@ +3522,3 @@
+
+    // Increment the number of patterns covered by the cases in the group.
+    if (CaseMatchesCondPattern)
----------------
Isn't it the number of cases covered by the "pattern", rather than the other way around?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3709
@@ +3708,3 @@
+
+    if (Succ == Destination)
+      continue;
----------------
It's not clear from just reading the function what Destination is.  It might be clearer to just use PHI->gerParent().

================
Comment at: test/Transforms/SimplifyCFG/switch-to-select-bitpattern.ll:95
@@ +94,2 @@
+  ret i32 %retval.0
+}
----------------
kariddi wrote:
> hans wrote:
> > would be nice to have tests for both the default with unreachable instruction, and the clever case where we know enough bits to figure out the default case can never happen.
> There is a problem actually with that.
> 
> If the default block is explicitly unreachable then the Unreachable case elimination (an optimization run earlier) substitutes the default with one of the cases.
> This might move one of the case group results from the UniqueResult vector to the DefaultResult.
> The UniqueResult vector reduces its size from 2 to 1 and the optimization bails out.
> 
> Actually there could be a way maybe to improve it to cover also this case ...
> In the case we only have one result in UniqueResult then the SelectBitmask is just the KnownOnes for that group.
> If we can determine that the cases in the result group are all and the only possible case values for the switch condition that have Ones in that position then we can use the KnownOnes as the bit mask.
> 
> So , in this example:
> int foo3_explicit_without_default(int a) {
>   a &= 0x6;
>   switch (a) {
>     case 0:
>       return 1;
>     case 2:
>       return 2;
>     case 4:
>       return 1;
>     case 6:
>       return 2;
>   }
>   __builtin_unreachable();
> }
> 
> We have two groups, one generating the value "2" and one generating the value "1". The default is unreachable.
> 
> after optimization becomes:
> 
> define i32 @foo3_explicit_without_default(i32 %a) nounwind readnone ssp uwtable {
>   %1 = and i32 %a, 6
>   switch i32 %1, label %4 [
>     i32 6, label %3
>     i32 2, label %2
>   ]
> 
> ; <label>:2                                       ; preds = %0
>   br label %4
> 
> ; <label>:3                                       ; preds = %0
>   br label %4
> 
> ; <label>:4                                       ; preds = %0, %3, %2
>   %.0 = phi i32 [ 2, %3 ], [ 2, %2 ], [ 1, %0 ]
>   ret i32 %.0
> }
> 
> Which is not optimizable, because the group generating "1" has been moved to the default.
> But , because:
> - KnownOnes for the cases generating "2" is 0x4, and
> - the possible bit patterns for the switch condition are only 0x0, 0x2, 0x4, 0x6 and
> - The cases in the group generating "2" are all and the only valid cases that match the KnownOnes mask for "2".
> Then that mask can be used to differentiate between the default and the group generating "2".
> 
> Should I modify the algorithm to match this case in this patch? Or is it better to do it separately? (It would involve changing everything to support UniqueResults.size() < 2).
Sounds like that should be done separately.

Can we just add a test for the case the default case is not explicitly unreachable, but we figure it out based on known bits of the condition and number of cases?

Actually, couldn't the unreachable case elimination have that logic instead of our transform?

http://reviews.llvm.org/D5525