[PATCH] Switch to Select optimisations

Hans Wennborg hans at chromium.org
Mon Sep 29 18:23:33 PDT 2014


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3487
@@ +3486,3 @@
+  UnreachableInst *UI = dyn_cast<UnreachableInst>(DefaultDest->getTerminator());
+  if ((DefaultResult == nullptr && !(UI && UI->getParent()->size() == 1)))
+    return false;
----------------
Instead of checking the size, I would probably just grab for the first instruction and check if that's an UnreachableInst:

  UnreachableInst *UI = dyn_cast<UnreachableInst>(DefaultDest->getFirstNonPHIOrDbg());

Actually, maybe this can just be baked into the if below:

  if (!DefaultResult && !isa<UnreachableInst>(DefaultDest->getFirstNonPHIOrDbg())

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3466
@@ +3465,3 @@
+    auto ResultFound = 
+      std::find_if(UniqueResults.begin(), UniqueResults.end(),
+                   [&Result](const ResultVectorTy::value_type &t) {
----------------
kariddi wrote:
> hans wrote:
> > If I understand correctly, UniqueResults is a mapping from result to vector of cases. Could we use a DenseMap instead and save some code here?
> The main problem of DenseMap is that it is unordered. We need to enforce the same order every run if we want repeatability of output. Using a vector here makes it easier and probably faster, because the typical size of the vector here is in that spot where actually linear scanning a vector is usually faster.
That's a good point. I wish the code was simpler here, though. It seems like what we want is really just 

  UniqueResults[Results[0]].push_back(CaseVal);

Maybe if we break out the code to a helper function, "addUniqueResult" or something?

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:3617
@@ +3616,3 @@
+
+// ConvertBitPatternSwitch - Helper function that checks if it is possible to
+// transform a switch with cases that are groupable by some bit pattern into
----------------
kariddi wrote:
> hans wrote:
> > I haven't had time to look at this one yet.
> > 
> > Do you think the bit pattern variant could be broken out to a separate patch?
> Mmm, this is a rather important optimization for the use case I'm trying to cover. Do you think it is really necessary to put it in a separate patch later on?
It's just that a bigger patch takes longer to review. I haven't looked at this part yet, but will try to do so as soon as I can.

http://reviews.llvm.org/D5525






More information about the llvm-commits mailing list