[PATCH] Switch to Select optimisations

[Hans Wennborg]

I think I understand the "bit pattern switch" a little better now, but it's not easy to understand from the code. I've tried to make some suggestions to make it more clear.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:80
@@ +79,3 @@
+    ResultVectorTy;
+  typedef SmallVector<std::pair<PHINode *, Constant *>, 4> ResultsTy;
+
----------------
If we're going to expose these typedef's throughout the whole file, they need less generic names.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3436
@@ +3435,3 @@
+static void addResult(ConstantInt *CaseVal,
+    ResultVectorTy &UniqueResults, const ResultsTy &Results) {
+  auto &Result = *Results.begin();
----------------
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.

================
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);
+  }
----------------
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)));

================
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,
----------------
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.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3592
@@ +3591,3 @@
+                   ConstantInt *MaxCaseSecond, IRBuilder<> &Builder) {
+  const bool FirstIsBigger =
+      MaxCaseSecond->getValue().slt(MinCaseFirst->getValue());
----------------
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());

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3645
@@ +3644,3 @@
+    KnownOneFirst &= KnownZeroSecond;
+    KnownOneSecond &= KnownZeroFirst;
+    // The comparison checks for ones that are always present in
----------------
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;

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

================
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);
----------------
Just to make the code easier to follow, I'd spell out "Compute known bits of the Condition." in a comment.

================
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();
----------------
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();

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

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3739
@@ +3738,3 @@
+  bool DefaultCanTrigger;
+  APInt KnownZeroFirst = APInt::getAllOnesValue(Bits),
+        KnownOneFirst = APInt::getAllOnesValue(Bits),
----------------
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.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3743
@@ +3742,3 @@
+      KnownOneSecond = APInt::getAllOnesValue(Bits);
+  unsigned FirstPossibleHits = 0, SecondPossibleHits = 0;
+  ConstantInt *MinCaseFirst = nullptr, *MinCaseSecond = nullptr,
----------------
it's not clear to me what a "hit" is

http://reviews.llvm.org/D5525