[PATCH] D60673: [SimplifyCFG] Improove and speed up ReduceSwitchRange

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Can you please write a human-readable description for the differential, in layman terms, that makes sense?
Rough bullet point list could be:
"Motivation. Issues with current implementations. The possible solutions. The implemented solution. Drawbacks, gotchas of the change.", etc.

This change is pretty big. It would be really best to do this in smaller steps.
Test coverage in particular is bothering me - this only has basic positive tests.
No negative tests. No tests for boundary conditions



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5131
                        const SmallDenseMap<PHINode *, Type *> &ResultTypes) {
-  if (SI->getNumCases() > TableSize || TableSize >= UINT64_MAX / 10)
+  if (SI->getNumCases() > TableSize || TableSize >= UINT64_MAX / 24)
     return false; // TableSize overflowed, or mul below might overflow.
----------------
What is this 24, 3*8? Make it variable with a name.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5177
+    // Table Size, including empty space, plus header size
+        SI->getNumCases() * 14) // size of cmp jmp mov ret
+    return true;
----------------
This sounds like a hardcoded value for some specific target.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5323-5326
+    if (CaseVal->getValue().ult(MinCaseVal->getValue()))
       MinCaseVal = CaseVal;
-    if (CaseVal->getValue().sgt(MaxCaseVal->getValue()))
+    if (CaseVal->getValue().ugt(MaxCaseVal->getValue()))
       MaxCaseVal = CaseVal;
----------------
Why is this being changed? Can this be a separate patch?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5392-5393
+    Value *V;
+    if (match(SI->getCondition(), m_c_Add(m_Value(V), m_ConstantInt(CAdd))) ||
+        match(SI->getCondition(), m_Xor(m_Value(V), m_ConstantInt(CXor)))) {
+      for (auto Case : SI->cases()) {
----------------
Constants are always on RHS anyway


================
Comment at: test/Transforms/SimplifyCFG/switch-simplify-range.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -simplifycfg -O2 < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
Please don't use `-O?` (or for that matter, don't passes other than the pass from directory name)
in tests outside of `PassOrdering` directory.


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

https://reviews.llvm.org/D60673