[PATCH] D21291: [SimplifyCFG] Range reduce switches

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4989
@@ +4988,3 @@
+  for (auto &C : SI->cases())
+    Values.push_back(C.getCaseValue()->getValue().getLimitedValue());
+  std::sort(Values.begin(), Values.end());
----------------
I'd prefer a `getZExtValue` or `getSExtValue` here.  That way the code will assert if someone later accidentally removed the check on the bit width.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4992
@@ +4991,3 @@
+
+  bool HasHoles = Values.back() != SI->getNumCases() - 1;
+  if (!HasHoles)
----------------
How about `{-2, 0, 2, 4}`?  That's sparse and can benefit from this pass, but here you'll conclude that it has no holes.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5006
@@ +5005,3 @@
+  unsigned I = 0;
+  for (auto &V : Values)
+    if (V / Divisor != I++)
----------------
Don't you need to check that `V` is divisible by `Divisor` here?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5007
@@ +5006,3 @@
+  for (auto &V : Values)
+    if (V / Divisor != I++)
+      // The switch still has a hole.
----------------
Might be worth calling out the signedness of the division here explicitly.  I'd mildly prefer bitwise ops instead of division.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5017
@@ +5016,3 @@
+  auto *Sub = Builder.CreateSub(SI->getCondition(), ConstantInt::get(Ty, Base));
+  auto *Div = Builder.CreateUDiv(Sub, ConstantInt::get(Ty, Divisor));
+  auto *Rem = Builder.CreateURem(Sub, ConstantInt::get(Ty, Divisor));
----------------
I'd be tempted to emit the bitwise ops here directly.  That way you don't do extra work later and are less sensitive to pass ordering.


Repository:
  rL LLVM

http://reviews.llvm.org/D21291