[PATCH] D52707: Switch optimization in IR for known maximum switch values

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Thanks for doing this! I have some comments, and as was pointed out, this needs tests.



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4844
+// destination as unreachable.
+static bool convertFiniteDefaultValuesToCases(SwitchInst *SI,
+                                              const DataLayout &DL,
----------------
The way I think about this is that it "widens" the switch to cover all input values. So maybe call the function something like that?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4859
+      (Known.countMinLeadingZeros() + Known.countMaxTrailingOnes()) ==
+         ValueWidth)
+    MaxSwitchValue = (~Known.Zero);
----------------
I think the following code would be easier to follow if you do these "known value is not negative etc." checks and then return false if they don't pass. Same thing for the unreachable default check: if that's true, there's no point in continuing.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4870
+      SI->getNumCases() > 0  &&  !UnreachableDefault  &&
+      NumCases.roundToDouble() / MaxSwitchValue.roundToDouble() > 0.70) {
+    // Here we care for only those values that are beyond the highest case
----------------
Instead of using floating point for this, how about something like "NumCases * 100 / MaxSwitchValue >= 70"?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4873
+    // value and up to the MaxSwitchValue.  The other missing values (holes)
+    // get automatically filled up during lowering.
+    BasicBlock *DefaultBlock = SI->getDefaultDest();
----------------
I think we need to care about the holes too, because we're going to mark the current default unreachable..


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4884
+                         DefaultResultsList, DL, TTI))
+      return false;
+
----------------
What's happening here? Why do we need to find a constant value on the phi node?

We'll need to update any phi node in the default block when adding the new case, but there's no need for it to be constant.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4901
+
+    for (int i = MaxCaseVal->getSExtValue() + 1;  MaxSwitchValue.uge(i);  i++) {
+      const APInt Val(ValueWidth, i, true);
----------------
I don't think this is enough. I think we need to fill the "holes" too.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4913
+
+    BasicBlock *NewDefaultBlock;
+    NewDefaultBlock = BasicBlock::Create(DefaultBlock->getContext(),
----------------
This needs a comment explaining that the new default is unreachable because the switch now has cases for all possible inputs.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:4921
+    SI->setDefaultDest(NewDefaultBlock);
+    return true;
+  }
----------------
Let's add a statistic variable to count how often this transformation happens (see e.g. NumLinearMaps).


https://reviews.llvm.org/D52707