<div dir="auto">Hi Shawn,<div dir="auto"><br></div><div dir="auto">Phab really isn't a replacement for git. Its a good idea to squash your commits locally so reviewers know what they're supposed to be doing and digging against. </div><div dir="auto"><br></div><div dir="auto">Cheers,</div><div dir="auto"><br></div><div dir="auto">James</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 26 May 2019, 16:49 Shawn Landden via Phabricator, <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">shawnl updated this revision to Diff 201451.<br>
shawnl added a comment.<br>
<br>
use saturating multiply<br>
<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D62465/new/" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D62465/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D62465" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D62465</a><br>
<br>
Files:<br>
  lib/Transforms/Utils/SimplifyCFG.cpp<br>
<br>
<br>
Index: lib/Transforms/Utils/SimplifyCFG.cpp<br>
===================================================================<br>
--- lib/Transforms/Utils/SimplifyCFG.cpp<br>
+++ lib/Transforms/Utils/SimplifyCFG.cpp<br>
@@ -5519,9 +5519,9 @@<br>
 static bool ReduceSwitchRange(SwitchInst *SI, IRBuilder<> &Builder,<br>
                               const DataLayout &DL,<br>
                               const TargetTransformInfo &TTI) {<br>
-  // The number of cases that need to be removed by a subtraction operation<br>
-  // to make it worth using.<br>
-  const unsigned SubThreshold = (SI->getFunction()->hasOptSize() ? 2 : 8);<br>
+  // The number of cases that need to be removed by a subtraction or<br>
+  // shift right operation to make it worth using.<br>
+  const unsigned Threshold = (SI->getFunction()->hasOptSize() ? 2 : 8);<br>
   auto *CondTy = cast<IntegerType>(SI->getCondition()->getType());<br>
   unsigned BitWidth = CondTy->getIntegerBitWidth();<br>
   if (BitWidth > 64 || !DL.fitsInLegalInteger(BitWidth))<br>
@@ -5569,25 +5569,29 @@<br>
     Shift = std::min(Shift, countTrailingZeros(V ^ BestIndexXor));<br>
   assert(Shift < 64);<br>
   if (Shift > 0) {<br>
-    MadeChanges = true;<br>
-    for (auto &V : Values)<br>
-      V >>= Shift;<br>
+    if (SaturatingMultiply<uint64_t>((1ULL << Shift) - 1, Values.size()) >= Threshold) {<br>
+      MadeChanges = true;<br>
+      for (auto &V : Values)<br>
+        V >>= Shift;<br>
+    } else<br>
+      // Not worth it.<br>
+      Shift = 0;<br>
   }<br>
<br>
   // We Xor against Values[] (any element will do) because the if we do not<br>
-  // start at zero, but also don't meet the SubThreshold, then we still might<br>
+  // start at zero, but also don't meet the Threshold, then we still might<br>
   // share common rights bits, and if this transform succeeds<br>
   // then we should insert the subtraction anyways, because the rotate trick<br>
   // below to avoid a branch needs the shifted away bits to be zero.<br>
<br>
   // Now transform the values such that they start at zero and ascend. Do not<br>
-  // do this if the shift reduces the lowest value to less than SubThreshold,<br>
-  // or if the subtraction is less than SubThreshold and it does not enable a<br>
+  // do this if the shift reduces the lowest value to less than Threshold,<br>
+  // or if the subtraction is less than Threshold and it does not enable a<br>
   // rotate.<br>
   uint64_t Base = 0;<br>
-  if ((BestIndexXor >= SubThreshold && Shift == 0) ||<br>
+  if ((BestIndexXor >= Threshold && Shift == 0) ||<br>
       (Shift > countTrailingZeros(BestIndexXor) &&<br>
-       Values[BestIndex] >= SubThreshold)) {<br>
+       Values[BestIndex] >= Threshold)) {<br>
     Base = BestIndexXor;<br>
     MadeChanges = true;<br>
     for (auto &V : Values)<br>
<br>
<br>
</blockquote></div>