[PATCH] D122485: [SimplifyCFG] Try to fold switch with single result value and power-of-2 cases to mask+select

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - I made a few more minor comments.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5717-5719
+    // case 0,4(bit 1 group)         -> select !!(and -5), result, default
+    // case 0,2,4,6(bit 0, 1 group)  -> select !!(and -7), result, default
+    // case 0,2,8,10(bit 0, 2 group) -> select !!(and -11), result, default
----------------
The bit numbering is not clear to me. "0,4" clears bit 2?
It might help to show the mask as a bit value (for example -5 is "0b1..1011").
Maybe better to spell out the comparison too:
"Cond & 0b1..1011 == 0 ? result : default"


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5722
+      ConstantInt *MinCaseVal = CaseValues[0];
+      APInt BitMask = APInt::getZero(MinCaseVal->getBitWidth());
+      // Find mininal value.
----------------
Move this variable declaration/initialization to just above the loop where it is filled in.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5730-5732
+      for (auto Case : CaseValues) {
+        BitMask |= (Case->getValue() - MinCaseVal->getValue());
+      }
----------------
Remove unnecessary braces around 1-line loop - { }.


================
Comment at: llvm/test/Transforms/SimplifyCFG/switch-to-select-two-case.ll:267
 
 define i8 @switch_to_select_two_case_results_no_default(i32 %i) {
 ; CHECK-LABEL: @switch_to_select_two_case_results_no_default(
----------------
It is independent of this patch, but we should put a TODO comment on this test or in the code because we do not produce the optimal code when there is no default:
https://alive2.llvm.org/ce/z/7qxumF


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122485/new/

https://reviews.llvm.org/D122485