[PATCH] D122485: [SimplifyCFG] Fold mutil cases to And mask

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D122485#3448597 <https://reviews.llvm.org/D122485#3448597>, @bcl5980 wrote:

> Try two patterns on current backend, this patch's implementation generate better asm on x86 and aarch64: https://godbolt.org/z/nsoWa7Kqr

Thanks for checking that. I suspect that a target that has condition-logic instructions (like PowerPC) might be better off with the icmp pattern for some examples, but I agree that we can default to the 'and' pattern here. It is shorter IR for the case with no case minimum offset.
I added some more tests and tried to clean up the existing code a bit more. Please rebase and address minor issues. Then I think this patch will be good to go.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5691
 // }
-// TODO: Handle switches with more than 2 cases that map to the same result.
+// 2. n bits group cases map to the same result into a select.
+// case 0, 4        (bit 1 group)        -> select !!(and -5), result, default
----------------
I think it would be better to put these examples closer to the code that does the transform.
I moved the existing example comment with:
0ef46dc0f9f3


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5695
+// case 0, 2, 8, 10 (bit 0, bit 2 group) -> select !!(and -11), result, default
+// TODO: If minimal case value is not 0 and only two cases have the same
+// value, both of two transform get 4 IR instructions with similar perf
----------------
Remove the TODO - if we are going to use the new code on the 2-case pattern, then it is really a TODO for instcombine and/or codegen to alter it if needed.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5706
       ResultVector[1].second.size() == 1) {
+    // If we are selecting between only two cases transform into a simple
+    // select or a two-way select if default is possible.
----------------
Don't move this comment.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5726
+  if (ResultVector.size() == 1 && DefaultResult) {
+    unsigned CaseCount = ResultVector[0].second.size();
+    if (isPowerOf2_32(CaseCount)) {
----------------
I added:
    ArrayRef<ConstantInt *> CaseValues = ResultVector[0].second;
to make this more readable. You can use that in the new code.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5730
+      APInt BitMask = APInt::getZero(MinCaseVal->getBitWidth());
+      // find mininal value
+      for (auto Case : ResultVector[0].second) {
----------------
Use complete sentence/punctuation for code comments:
  // Find minimal value.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5736
+
+      // Mark the bits case number touched
+      for (auto Case : ResultVector[0].second) {
----------------
Add period at end of sentence.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5741-5742
+
+      // Check if cases with the same result can cover all number
+      // in touched bits
+      if (BitMask.countPopulation() == Log2_32(CaseCount)) {
----------------
Add period at end of sentence.


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

https://reviews.llvm.org/D122485