[PATCH] Switch to Select optimisations

[Marcello Maggioni]

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3436
@@ +3435,3 @@
+static void addResult(ConstantInt *CaseVal,
+    ResultVectorTy &UniqueResults, const ResultsTy &Results) {
+  auto &Result = *Results.begin();
----------------
hans wrote:
> I would just take the "Constant *Result" as a parameter.
> 
> It would be nice to have a comment explaining what the function does, and that would also be a good place to the mapping that UniqueResults is.
Done.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3448
@@ +3447,3 @@
+    // Avoid querying the map again if we already found the result.
+    ResultFound->second.push_back(CaseVal);
+  }
----------------
hans wrote:
> I know I'm old-school, but with less fancy C++, I think the code would become more clear:
> 
>   for (auto &I : UniqueResults) {
>     if (I.first == Result) {
>       I.second.push_back(CaseVal)
>       return;
>     }
>   }
>   UniqueResults.push_back(std::make_pair(Result, SmallVector<ConstantInt*, 4>(1, CaseVal)));
I think you are right :) Old school seems cleaner here!

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3499
@@ +3498,3 @@
+// cases, like known ones and zeroes, the minimum and the maximum case and
+// the number of case patterns that match the bitmask of the switch value.
+static void AnalyzeSwitchCases(const SmallVectorImpl<ConstantInt *> &Cases,
----------------
hans wrote:
> It would be nice with a comment about what "known ones and zeroes" mean. Maybe here, or when they're computed further down in the function.
> 
> Actually, maybe the parameters need better names. I commented about this further down.
Done.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3592
@@ +3591,3 @@
+                   ConstantInt *MaxCaseSecond, IRBuilder<> &Builder) {
+  const bool FirstIsBigger =
+      MaxCaseSecond->getValue().slt(MinCaseFirst->getValue());
----------------
hans wrote:
> I think I commented on these before. It's just that with a name such as "first is bigger", I'd find it natural if the code to say that too, i.e.
> 
>   FirstIsBigger = MinCaseFirst->getValue().sgt(MaxCaseSecond->getValue());
Done :)

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3645
@@ +3644,3 @@
+    KnownOneFirst &= KnownZeroSecond;
+    KnownOneSecond &= KnownZeroFirst;
+    // The comparison checks for ones that are always present in
----------------
hans wrote:
> These are the same values that were computed in the if statements above. Rather than repurposing these variables, I'd hoist this to above the if statement and try to give them better name and comment (it's not obvious to me what "unambigous patterns" mean).
> 
> Maybe (but good names are hard):
> 
>   APInt FirstRangeSelectBits = FirstRangeKnownOneBits & SecondRangeKnownZeroBits;
>   APInt SecondRangeSelectBits = SecondRangeKnownOneBits & FirstRangeKnownZeroBits;
>   if (!DefaultCanTrigger && (FirstRangeSelectBits || SecondRangeSelectBits) {
>     ...
>   }
> 
> Actually, since we don't do anything interesting if DefaultCanTrigger is true, we could just bail out early for that case - near the top:
> 
>   if (DefaultCanTrigger)
>     return nullptr;
Fixed :)

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3717
@@ +3716,3 @@
+
+  const unsigned Bits = Cond->getType()->getIntegerBitWidth();
+
----------------
hans wrote:
> Since this ins't a bitmask, but a width, I would call it BitWidth, or CondWidth or something.
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3719
@@ +3718,3 @@
+
+  APInt KnownZeroCond(Bits, 0), KnownOneCond(Bits, 0);
+  computeKnownBits(Cond, KnownZeroCond, KnownOneCond, DL, 0, AT, SI);
----------------
hans wrote:
> Just to make the code easier to follow, I'd spell out "Compute known bits of the Condition." in a comment.
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3722
@@ +3721,3 @@
+  // Number of bits that are fixed in the switch condition value.
+  const unsigned CondStuckBits =
+      (KnownZeroCond | KnownOneCond).countPopulation();
----------------
hans wrote:
> I'd use the terminaly "known" instead of fixed or stuck. I'd also prefix the variable with Num since it's a count, not mask - something like:
> 
>   const unsigend NumKnownBits = (KnownCondZeroBits | KnownCondOneBits).countPopulation();
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3724
@@ +3723,3 @@
+      (KnownZeroCond | KnownOneCond).countPopulation();
+  Value *SelectValue = nullptr;
+
----------------
hans wrote:
> I'd just declare this when it's first used below.
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3739
@@ +3738,3 @@
+  bool DefaultCanTrigger;
+  APInt KnownZeroFirst = APInt::getAllOnesValue(Bits),
+        KnownOneFirst = APInt::getAllOnesValue(Bits),
----------------
hans wrote:
> I find these names a little bit cryptic. What do you think about something like FirstRangeKnownZeroBits. Though it's a bit longer, it makes the contents clearer.
> 
> Also, since these declarators are all on separate lines anyway, I'd just write them out as one declaration per line.
Done

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3743
@@ +3742,3 @@
+      KnownOneSecond = APInt::getAllOnesValue(Bits);
+  unsigned FirstPossibleHits = 0, SecondPossibleHits = 0;
+  ConstantInt *MinCaseFirst = nullptr, *MinCaseSecond = nullptr,
----------------
hans wrote:
> it's not clear to me what a "hit" is
I changed the name to something that is a little bit more similar to what it actually is

http://reviews.llvm.org/D5525