[PATCH] D60982: [SimplifyCFG] Use lookup tables when they are more space efficient or a huge speed win.

[James Molloy via Phabricator via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Please reupload with full context as described in the developer's guide.

> These numbers will change in a later patch, (when sparse maps make switch much more compact) so we shouldn't argue too much about this cut-off.

Are you referring to your changes in builtins/? If so, that's surely part of compiler-rt, and you can't expect targets to have that available at runtime.



================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5272
 
+// FIXME Move this function up here when commiting. This makes the patch easier to read.
+static bool ReduceSwitchRange(SwitchInst *SI, IRBuilder<> &Builder,
----------------
Please upload the code for review as it will be committed.


================
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 / (3 * 8))
     return false; // TableSize overflowed, or mul below might overflow.
----------------
> ( 3 * 8 )

... but why?  This isn't obvious from the code so needs comments. Note that the original code randomly dividing by 10 was just as bad, but you've clearly got a good reason for this change so should document it ;)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5135
+  // If the table only contains i8 or smaller condition, it has a bounded size of
+  // 256 times the largest legal size, and will be more performant with a lookup table.
+  if (!SI->getFunction()->hasOptSize() &&
----------------
> will

Bold claim :) 


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5142
   bool HasIllegalType = false;
+  unsigned BiggestTypeSize = 0;
   for (const auto &I : ResultTypes) {
----------------
Nit: please use the term "Largest". They're synonymous and I know this is nitpicking, but it's the more generally used term.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5145
     Type *Ty = I.second;
+    unsigned TySize = DL.getTypeAllocSize(Ty);
 
----------------
This is only used in one place; please fold into its use.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5173
 
-  // The table density should be at least 40%. This is the same criterion as for
-  // jump tables, see SelectionDAGBuilder::handleJTSwitchCase.
+  // If the table is smaller, always use it
+  if (TableSize * BiggestTypeSize + 14 <
----------------
Please describe this heuristic in more detail. Why is it always good?

Take a look at the line 5162 in the diffbase for a good example of a heuristic description. It captures what the criterion is, and a rationale for it.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5174
+  // If the table is smaller, always use it
+  if (TableSize * BiggestTypeSize + 14 <
+    // Table Size, including empty space, plus header size
----------------
These magic numbers have no place in SimplifyCFG. If you need to be this accurate, add a TTI hook.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5183
+
+  // The table density should be at least 33% for 64-bit integers.
   // FIXME: Find the best cut-off.
----------------
But why? You've replaced 40% with 33% and you mention 64-bit integers but the following heuristic doesn't use 64-bit integers anywhere.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5386
+    Value *V;
+    if (match(SI->getCondition(), m_Add(m_Value(V), m_ConstantInt(CAdd))) ||
+        match(SI->getCondition(), m_Sub(m_Value(V), m_ConstantInt(CSub))) ||
----------------
This assumes whatever constant add/sub/xor was planted still exists. There's no guarantee of that; for example if V is a constant, IRBuilder would have constant folded immediately.

In general unstitching code like this is inherently problematic. Note that isn't the same as planting an inverting code sequence - that's fine. We can use the optimizer to clean the duplicated stuff up and everyone is happy. That is much preferable to hamhandedly unstitching stuff and looking for patterns of code you've just emitted. It also adds an implicit contract between different parts of SimplifyCFG that I guarantee someone will miss when they update this code :)


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5400
+    }
+  // Call this from in here, because we need the context necessary for this if/else
+  } else if (ReduceSwitchRange(SI, Builder, DL, TTI))
----------------
This comment doesn't parse for me.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5401
+  // Call this from in here, because we need the context necessary for this if/else
+  } else if (ReduceSwitchRange(SI, Builder, DL, TTI))
+    return true; // We will get called again
----------------
This code structure is a little hard to understand. Instead of trying to tack onto the end of the if statement to reuse the heuristic, can you extract the heuristic into a bool and reuse it explicitly here? Then the reader doesn't need to think about context.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:5713
 
+  // This is also called within SwitchToLookupTable
+  if (ReduceSwitchRange(SI, Builder, DL, TTI))
----------------
It's not clear to me why this matters from this context. Perhaps if you wrote something like:

// Call ReduceSwitchRange *after* SwitchToLookupTable as SwitchToLookupTable calls this internally.



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

https://reviews.llvm.org/D60982