[PATCH] D21291: [SimplifyCFG] Range reduce switches

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks, I like this :-) Just some nits.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5044
@@ +5043,3 @@
+  uint64_t Diff;
+  if (Values.front() <= Values.back())
+    Diff = (uint64_t)Values.back() - (uint64_t)Values.front();
----------------
Aren't Values sorted (from line 5081), so this should be unnecessary?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5067
@@ +5066,3 @@
+                              const TargetTransformInfo &TTI) {
+  if (SI->getCondition()->getType()->getIntegerBitWidth() > 64)
+    return false;
----------------
Maybe we should check DL.fitsInLegalInteger() instead of hard-coding 64 here.

Oh I see, 64 is because we use [u]int64_t below. It might still be a good idea to check fitsInLegalInteger() so we're not creating a non-legal rotate operation.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5088
@@ +5087,3 @@
+  // First, transform the values such that they start at zero and ascend.
+  uint64_t Base = Values[0];
+  for (auto &V : Values)
----------------
why isn't Base int64_t already? Also, could the subtraction overflow?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5106
@@ +5105,3 @@
+  // be beneficial - flag values are often powers of two and we could use a CLZ
+  // as the key function.
+  if (GCD <= 1 || !llvm::isPowerOf2_64(GCD))
----------------
Any bijective transformation of the case values will work. The problem is just deciding when they're profitable, I suppose. But I'm all for more creative switch lowerings :-)


Repository:
  rL LLVM

https://reviews.llvm.org/D21291